[WB-1871] IconButton: Rename color prop to actionType
#2510
Conversation
…tionType`. Also rename the `default` value to `progressive`.
🦋 Changeset detectedLatest commit: d5ebb1e 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: -37 B (-0.04%) Total Size: 98.8 kB
ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-zqttvbhhap.chromatic.com/ Chromatic results:
|
There was a problem hiding this comment.
note: Simplified argTypes here to rely on the code comments directly (instead of duplicating the description here).
| disabled: boolean, | ||
| kind: Kind, | ||
| light: boolean, | ||
| theme: IconButtonThemeContract, | ||
| ) { | ||
| let actionType: ActionType = getActionType(buttonColor, disabled); | ||
| const themeVariant = theme.color[kind][actionType]; | ||
| let actionTypeOrDisabled: ActionType = disabled ? "disabled" : actionType; |
There was a problem hiding this comment.
note: This replaces the logic of getActionType() now that we have a correct mapping. This is just to add support to the disabled variant.
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 (52d08e8) 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="PR2510"Packages can also be installed manually by running: pnpm add @khanacademy/wonder-blocks-<package-name>@PR2510 |
There was a problem hiding this comment.
This looks good to me, but I'll be curious to hear @beaesguerra's thoughts! I think it will clarify how color is used in themes. It was a bit confusing to me before with the levels of tokens and configuration objects.
My only other thought is: should there be tests for these changes, or does Storybook cover it?
@marcysutton Great question! We have been moving a lot of unit tests that covered styling to Visual regression testing with Storybook + Chromatic, and in this case it would be covered by these tests: |
There was a problem hiding this comment.
Looks great! Glad we're also moving towards semantic naming for props too!
Few follow up questions:
- Are there plans to also update perseus before webapp since it's a breaking change?
- Will we still also be updating the icon semantic tokens (
primarytoneutral, getting rid of thesecondarysemantic icon token, etc)? And having thekindprop values match those values? (related to the thread from last week!)
That's correct. I'll chat with the LEMS team before landing this change (sometime next week).
Yes, but that will happen in an upcoming PR: https://khanacademy.atlassian.net/browse/WB-1902 I'm going to add an "Implementation plan" to this PR and the stacked ones to make the plan clearer. Thanks for asking these questions @beaesguerra! |
Summary:
We are trying to get more alignment with design, and part of that is that icon
buttons will use an
actionTypeprop, which supportsprogressiveanddestructive.This PR includes the following API changes:
color=>actionType.color=default=>actionType=progressive.NOTE:
progressiveis now the default value.Issue: https://khanacademy.atlassian.net/browse/WB-1871
Test plan:
Navigate to the IconButton docs and verify that the prop is now
actionTypeinstead of
color./?path=/docs/packages-iconbutton--docs#actiontype