Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix how mix from wonder-blocks-color formats single-digit color components #2065

Merged
merged 3 commits into from
Sep 26, 2023

Conversation

timmcca-be
Copy link
Contributor

@timmcca-be timmcca-be commented Sep 26, 2023

Summary:

Previously, if the red, green, or blue component was less than 16, its hex digit would be repeated (e.g. f would become ff). With this change, we preserve the real value of the number by prepending a 0 instead (e.g. f becomes 0f).

Test plan:

yarn run jest packages/wonder-blocks-color/src/util/__tests__/utils.test.ts

@timmcca-be timmcca-be self-assigned this Sep 26, 2023
@changeset-bot
Copy link

changeset-bot bot commented Sep 26, 2023

🦋 Changeset detected

Latest commit: f46f000

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 22 packages
Name Type
@khanacademy/wonder-blocks-color Major
@khanacademy/wonder-blocks-accordion Patch
@khanacademy/wonder-blocks-banner Patch
@khanacademy/wonder-blocks-birthday-picker Patch
@khanacademy/wonder-blocks-button Patch
@khanacademy/wonder-blocks-cell Patch
@khanacademy/wonder-blocks-clickable Patch
@khanacademy/wonder-blocks-dropdown Patch
@khanacademy/wonder-blocks-form Patch
@khanacademy/wonder-blocks-grid Patch
@khanacademy/wonder-blocks-icon-button Patch
@khanacademy/wonder-blocks-labeled-field Patch
@khanacademy/wonder-blocks-link Patch
@khanacademy/wonder-blocks-modal Patch
@khanacademy/wonder-blocks-pill Patch
@khanacademy/wonder-blocks-popover Patch
@khanacademy/wonder-blocks-progress-spinner Patch
@khanacademy/wonder-blocks-search-field Patch
@khanacademy/wonder-blocks-theming Patch
@khanacademy/wonder-blocks-toolbar Patch
@khanacademy/wonder-blocks-tooltip Patch
@khanacademy/wonder-blocks-switch Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@timmcca-be timmcca-be marked this pull request as ready for review September 26, 2023 16:37
@khan-actions-bot khan-actions-bot requested a review from a team September 26, 2023 16:38
@khan-actions-bot
Copy link
Contributor

khan-actions-bot commented Sep 26, 2023

Gerald

Required Reviewers
  • @Khan/wonder-blocks for changes to .changeset/afraid-rings-knock.md, packages/wonder-blocks-color/src/util/utils.ts, packages/wonder-blocks-color/src/util/__tests__/utils.test.ts

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

@timmcca-be timmcca-be changed the title Fix how mix from wonder-blocks-color returns single-digit color components Fix how mix from wonder-blocks-color formats single-digit color components Sep 26, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 26, 2023

Size Change: +3 B (0%)

Total Size: 89.7 kB

Filename Size Change
packages/wonder-blocks-color/dist/es/index.js 1.15 kB +3 B (0%)
ℹ️ View Unchanged
Filename Size
packages/wonder-blocks-accordion/dist/es/index.js 55 B
packages/wonder-blocks-banner/dist/es/index.js 3.06 kB
packages/wonder-blocks-birthday-picker/dist/es/index.js 1.69 kB
packages/wonder-blocks-breadcrumbs/dist/es/index.js 1.13 kB
packages/wonder-blocks-button/dist/es/index.js 3.98 kB
packages/wonder-blocks-cell/dist/es/index.js 2.18 kB
packages/wonder-blocks-clickable/dist/es/index.js 3.21 kB
packages/wonder-blocks-core/dist/es/index.js 3.67 kB
packages/wonder-blocks-data/dist/es/index.js 6.33 kB
packages/wonder-blocks-dropdown/dist/es/index.js 12 kB
packages/wonder-blocks-form/dist/es/index.js 5.42 kB
packages/wonder-blocks-grid/dist/es/index.js 1.36 kB
packages/wonder-blocks-i18n/dist/es/index.js 4.54 kB
packages/wonder-blocks-icon-button/dist/es/index.js 2.15 kB
packages/wonder-blocks-icon/dist/es/index.js 2.83 kB
packages/wonder-blocks-labeled-field/dist/es/index.js 72 B
packages/wonder-blocks-layout/dist/es/index.js 1.87 kB
packages/wonder-blocks-link/dist/es/index.js 3.03 kB
packages/wonder-blocks-modal/dist/es/index.js 5.02 kB
packages/wonder-blocks-pill/dist/es/index.js 1.03 kB
packages/wonder-blocks-popover/dist/es/index.js 4.31 kB
packages/wonder-blocks-progress-spinner/dist/es/index.js 1.51 kB
packages/wonder-blocks-search-field/dist/es/index.js 1.5 kB
packages/wonder-blocks-spacing/dist/es/index.js 158 B
packages/wonder-blocks-switch/dist/es/index.js 2.06 kB
packages/wonder-blocks-testing/dist/es/index.js 3.94 kB
packages/wonder-blocks-theming/dist/es/index.js 1.21 kB
packages/wonder-blocks-timing/dist/es/index.js 1.78 kB
packages/wonder-blocks-toolbar/dist/es/index.js 862 B
packages/wonder-blocks-tooltip/dist/es/index.js 5.05 kB
packages/wonder-blocks-typography/dist/es/index.js 1.49 kB

compressed-size-action

@codecov
Copy link

codecov bot commented Sep 26, 2023

Codecov Report

Merging #2065 (f46f000) into main (6514873) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2065      +/-   ##
==========================================
- Coverage   96.91%   96.91%   -0.01%     
==========================================
  Files         235      235              
  Lines       26493    26492       -1     
  Branches     2355     2307      -48     
==========================================
- Hits        25677    25676       -1     
  Misses        816      816              
Files Coverage Δ
packages/wonder-blocks-color/src/util/utils.ts 100.00% <100.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6514873...f46f000. Read the comment docs.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 26, 2023

A new build was pushed to Chromatic! 🚀

https://5e1bf4b385e3fb0020b7073c-keuymyxivf.chromatic.com/

Chromatic results:

Metric Total
Captured snapshots 0
Tests with visual changes 0
Total stories 360
Inherited (not captured) snapshots [TurboSnap] 305
Tests on the build 305

Copy link
Member

@somewhatabstract somewhatabstract left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, in the most part, though I think this should be a major bump given that it changes functionality.

In addition, the test plan make tesc doesn't make sense for Wonder Blocks as there is no make tesc. Should be yarn test or yarn jest packages/wonder-blocks-color, I believe.


My review uses Conventional Comments to add context and set expectations for the comments I am leaving.

@@ -0,0 +1,5 @@
---
"@khanacademy/wonder-blocks-color": patch
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This is a breaking change for code that relies on the old behavior. As such I think this likely needs to be a major change, though I could be persuaded down to minor since it is unlikely that there is code relying on this.

@@ -61,10 +61,9 @@ const format = (color: Color): string => {
const b = Math.round(color.b);

if (color.a === 1) {
// @ts-expect-error [FEI-5019] - TS7006 - Parameter 'c' implicitly has an 'any' type.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: Thanks for taking the time to get rid of an error suppression!

@timmcca-be timmcca-be merged commit 48d3c7e into main Sep 26, 2023
11 checks passed
@timmcca-be timmcca-be deleted the color-mix-fix branch September 26, 2023 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants