-
Notifications
You must be signed in to change notification settings - Fork 10
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
[WB-1814.6] Refactor Link to use semantic colors #2464
Conversation
🦋 Changeset detectedLatest commit: b8c74cd The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
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 |
Size Change: +125 B (+0.13%) Total Size: 97.4 kB
ℹ️ View Unchanged
|
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (7162551) and published all packages with changesets to npm. You can install the packages in webapp by running: ./services/static/dev/tools/deploy_wonder_blocks.js --tag="PR2464" Packages can also be installed manually by running: pnpm add @khanacademy/wonder-blocks-<package-name>@PR2464 |
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-nhdifluqzr.chromatic.com/ Chromatic results:
|
story: ``, | ||
}, | ||
}, | ||
export const Navigation: StoryComponentType = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: I messed up CSF here (from my previous PR), so I'm fixing it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes look good! Left some non-blocking questions in the PR, and one comment in Chromatic I'd like to get your thoughts on!
}, | ||
focus: { | ||
border: semanticColor.border.focus, | ||
foreground: action.hover.foreground, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this is an example where action.focus.foreground
would be useful! If we change hover styles, would we expect focus styles to change too?
foreground: color.purple, | ||
}, | ||
pressVisited: { | ||
foreground: mix(color.offBlack32, color.purple), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my own learning, how do we know when to use the color utils like mix
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH this shouldn't be used too much these days as most of the mixed colors are codified as primitive colors. This was originally introduced to mix colors in the past due to the limitation in our previous primitive colors, but now that we include more shades, this shouldn't be needed anymore (mostly).
const action = semanticColor.action.outlined.progressive; | ||
|
||
// NOTE: This color is only used here. | ||
const pink = "#fa50ae"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'll be nice when we have the full color palette so we won't have to hardcode one-off colors!
5ee46cd
to
6b2afa4
Compare
…anticColor` tokens intead of `color`. Also moved the link colors to an object to prepare this for an upcoming theme integration"
46edf73
to
b8c74cd
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2464 +/- ##
============================
============================
Continue to review full report in Codecov by Sentry.
|
## Summary: Next step is to refactor the `Modal` package to use semantic colors. Besides the migration, this PR also includes the following changes: - Reworked the theme structure to split tokens by sub-component. - Updated stories to use the new semantic colors. - Adapted some components to use theming instead of hardcoded colors. ### Implementation plan: 1. #2439 2. #2440 3. #2441 4. #2446 5. #2449 6. #2464 7. Modal (current PR) 8. Popover, Tooltip 9. Dropdown 10. Clickable, Pill, Toolbar Issue: WB-1814 ## Test plan: Verify that the Modal Chromatic snapshots are mostly unchanged. URL: `/?path=/docs/packages-modal-onepanedialog--docs&globals=viewport:desktop` Author: jandrade Reviewers: jandrade, beaesguerra, marcysutton Required Reviewers: Approved By: beaesguerra Checks: ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 20.x), ✅ Test / Test (ubuntu-latest, 20.x, 2/2), ✅ Lint / Lint (ubuntu-latest, 20.x), ✅ Test / Test (ubuntu-latest, 20.x, 1/2), ✅ Check build sizes (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Chromatic - Build and test on regular PRs / chromatic (ubuntu-latest, 20.x), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ⏭️ Chromatic - Skip on Release PR (changesets), ✅ gerald, ⏭️ dependabot Pull Request URL: #2468
…#2470) ## Summary: Next step is to refactor the `Popover`, `Tooltip` and `Toolbar` packages to use semantic colors. ### Implementation plan: 1. #2439 2. #2440 3. #2441 4. #2446 5. #2449 6. #2464 7. #2468 8. Popover, Tooltip, Toolbar (current PR) 9. Dropdown 10. Clickable, Pill Issue: WB-1814 ## Test plan: Verify that the Modal Chromatic snapshots are mostly unchanged. Author: jandrade Reviewers: beaesguerra Required Reviewers: Approved By: beaesguerra Checks: ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 20.x), ✅ Test / Test (ubuntu-latest, 20.x, 2/2), ✅ Test / Test (ubuntu-latest, 20.x, 1/2), ✅ Lint / Lint (ubuntu-latest, 20.x), ✅ Check build sizes (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Chromatic - Build and test on regular PRs / chromatic (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ⏭️ Chromatic - Skip on Release PR (changesets), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ✅ gerald, ⏭️ dependabot Pull Request URL: #2470
## Summary: - Migrate Clickable and Pill to use semantic colors - Add `All Variants` story to Pill ### Implementation plan: 1. #2439 2. #2440 3. #2441 4. #2446 5. #2449 6. #2464 7. #2468 8. #2470 9. Clickable, Pill (current PR) 10. Dropdown Issue: WB-1814 ## Test plan: Navigate to the `Clickable` and `Pill` stories in Storybook and verify that the colors are correct. Author: jandrade Reviewers: jandrade, beaesguerra Required Reviewers: Approved By: beaesguerra Checks: ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 20.x), ✅ Test / Test (ubuntu-latest, 20.x, 2/2), ✅ Test / Test (ubuntu-latest, 20.x, 1/2), ✅ Lint / Lint (ubuntu-latest, 20.x), ✅ Check build sizes (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Chromatic - Build and test on regular PRs / chromatic (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ⏭️ Chromatic - Skip on Release PR (changesets), ✅ gerald, ⏭️ dependabot Pull Request URL: #2472
…2474) ## Summary: Last PR that migrates the remaining components to use semantic colors. - Added `All Variants` stories to `ActionItem` and `OptionItem`. - Refactored `ActionItem` and `OptionItem` to use semantic colors. - Also refactored `ActionMenu`, `SingleSelect`, `MultiSelect` and `Combobox` to use semantic colors. - Simplified the styles in `ActionMenuOpenerCore` to use a single static StyleSheet instance (instead of generating another one in runtime). ### Implementation plan: 1. #2439 2. #2440 3. #2441 4. #2446 5. #2449 6. #2464 7. #2468 8. #2470 9. #2472 10. Dropdown (current PR) Issue: WB-1814 ## Test plan: Verify that all the dropdown stories are still working as expected. Author: jandrade Reviewers: jandrade, beaesguerra Required Reviewers: Approved By: beaesguerra Checks: ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 20.x), ✅ Lint / Lint (ubuntu-latest, 20.x), ✅ Test / Test (ubuntu-latest, 20.x, 2/2), ✅ Test / Test (ubuntu-latest, 20.x, 1/2), ✅ Check build sizes (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Chromatic - Build and test on regular PRs / chromatic (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ⏭️ Chromatic - Skip on Release PR (changesets), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ⏭️ dependabot, ✅ gerald Pull Request URL: #2474
Next step is to refactor the
Link
component to use semantic colors.Besides the migration, this PR also includes the following changes:
Implementation plan:
Issue: WB-1814
Test plan:
Verify that the Chromatic snapshots are unchanged.
URL:
/?path=/story/packages-link-link-all-variants--default