-
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.5] Refactor IconButton to use semantic colors #2449
Conversation
🦋 Changeset detectedLatest commit: e7b02f3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 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: +169 B (+0.17%) Total Size: 98.5 kB
ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-whryhodlfs.chromatic.com/ Chromatic results:
|
buttonColor: ButtonColor, | ||
disabled: boolean, | ||
kind: Kind, | ||
light: boolean, | ||
buttonColor: string, | ||
theme: IconButtonThemeContract, |
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: Just reordered the arguments alphabetically.
) { | ||
switch (kind) { |
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: Changed from switch
to if
as constants cannot be redefined within switch statements. I did this as I wanted to be able to reuse const themeVariant
etc for better readability.
} | ||
|
||
const _generateStyles = ( | ||
buttonColor = "default", | ||
kind: "primary" | "secondary" | "tertiary", | ||
buttonColor: ButtonColor = "default", |
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: Same here related to reordering args alphabetically.
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: Modified the structure to make it really closer to semanticColor.action
. Note that this time used primary
, secondary
, etc instead of filled,
outlinedto be able to map the keys to the props defined in
IconButton(e.g.
kind="primary"`). Still undecided with which approach to take. Any thoughts are completely welcome!
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 (7ed03e7) 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="PR2449" Packages can also be installed manually by running: yarn add @khanacademy/wonder-blocks-<package-name>@PR2449 |
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.
This looks like a really robust overhaul! I left some questions on things I wasn't understanding 100%. But I think it's going to be way easier to theme with this in place. Nice work!
autodocs: false, | ||
}, | ||
}, | ||
tags: ["!autodocs"], |
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.
praise: I didn't know you could do this! Nice!
@@ -101,6 +104,7 @@ const IconButtonCore: React.ForwardRefExoticComponent< | |||
const renderInner = (router: any): React.ReactNode => { | |||
const buttonStyles = _generateStyles( | |||
color, | |||
!!disabled, |
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.
question: Is the boolean coercion of disabled
meant to handle an undefined case?
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.
That's correct, this is mostly required b/c that prop is optional.
*/ | ||
function getActionType(buttonColor: ButtonColor, disabled: boolean) { | ||
const actionType = | ||
buttonColor === "destructive" ? "destructive" : "progressive"; |
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.
question: could you use nullish coalescing here? (out of curiosity)
const actionType = buttonColor === "destructive" ?? "progressive";
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.
In this case it wouldn't work because the left-hand operand would return true
if the condition is fulfilled.
const actionType = buttonColor === "destructive" ?? "progressive";
// when `buttonColor=destructive` => actionType=true
// when `buttonColor=default` => actionType="progressive"
Apologies if I'm being too literal 😅, but maybe I'm missing something in the expression (this could still use nullish coalescing here, which would be cool).
* will apply the same styles as Button. | ||
*/ | ||
const baseColorStates = { | ||
...semanticColor.action.outlined, |
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.
question: what case does this spread token cover? Could you add a comment for it?
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.
Good point! I'll add it.
@@ -18,6 +18,9 @@ import { | |||
IconButtonThemeContract, | |||
} from "../themes/themed-icon-button"; | |||
|
|||
type Kind = "primary" | "secondary" | "tertiary"; | |||
type ButtonColor = "default" | "destructive"; |
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.
question: Where does progressive
come in? Should that be included here among the options?
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.
ahh great question..... so the thing is that historically we have called the "blue" color version as default
, but this is changing with Polaris. This is a change that it is being proposed in Figma (which makes sense IMO). So we have to make that mapping (default
-> progressive
), but the goal is to align on the naming later. I'll add a note in the code to clarify that.
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.
I created a ticket to address this later. I'll add the TODO comment to point to it. Thanks again for raising this question.
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.
Looks good! Left a question around naming!
: theme.color.stroke.action.default; | ||
|
||
const buttonType = `${color}-${kind}-${light}-${size}-${themeName}`; | ||
const buttonType = `${buttonColor}-d_${disabled}-${kind}-l_${light}-${size}-${themeName}`; |
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.
question: What does the -d_
mean in the buttonType name?
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.
Disabled, but that's just to be able to cache this string properly.
This is mostly for debugging purposes if needed.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2449 +/- ##
============================
============================
Continue to review full report in Codecov by Sentry.
|
Next step is to refactor the `Link` component to use semantic colors. Besides the migration, this PR also includes the following changes: - Reworked the theme structure to make it closer to the semanticColor structure. - Updated stories to use the new semantic colors. ### Implementation plan: 1. #2439 2. #2440 3. #2441 4. #2446 5. #2449 6. Link (current PR) 7. Modal 8. Popover, Tooltip 9. Pill 10. Clickable, Toolbar Issue: WB-1814 ## Test plan: Verify that the Chromatic snapshots are unchanged. URL: `/?path=/story/packages-link-link-all-variants--default` 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), ⏭️ Chromatic - Skip on Release PR (changesets), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ⏭️ dependabot, ✅ gerald Pull Request URL: #2464
## 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
Summary:
Next step is to refactor the
IconButton
component to use semantic colors.Besides the migration, this PR also includes the following changes:
getStylesByKind
).border
to all the states to prepare the work for Polaris as theseicon buttons will look similar to the Button ones.
Implementation plan:
Issue: WB-1814
Test plan:
Verify that the Chromatic snapshots are unchanged.
URL:
/?path=/story/packages-iconbutton-all-variants--default