[WB-1852.2] Remove Button.light variant, modify actionStyles to support Button instances #2563
Conversation
… support Button instances
…ccount for `Button` instances
…no longer maintained. Simplifes pseudo-states (e.g. `:hover`, `:active`).
🦋 Changeset detectedLatest commit: 6d406a7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 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: -245 B (-0.24%) Total Size: 103 kB
ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-lpkevdotwa.chromatic.com/ Chromatic results:
|
There was a problem hiding this comment.
note: Cleaned up this file to remove all the light variants in Snapshots.
There was a problem hiding this comment.
note: Cleaned up the descriptions in this file now that ArgTypes are inferred better after the SB 8 migration.
| light: { | ||
| description: "Whether the button is on a dark/colored background.", | ||
| control: {type: "boolean"}, | ||
| table: { | ||
| category: "Theming", | ||
| type: { | ||
| summary: "boolean", | ||
| detail: "Sets primary button background color to white, and secondary and tertiary button title to color.", | ||
| }, |
There was a problem hiding this comment.
note: The only big change in this file was removing this argType as this light prop is now removed from the codebase.
| // NOTE: This is a workaround while we move | ||
| // this button to DAB and remove the | ||
| // `Button.khanmigo` theme. | ||
| { | ||
| background: "transparent", | ||
| ":hover": {background: "transparent"}, | ||
| }, |
There was a problem hiding this comment.
note: As mentioned in the comment, this will go away as soon as I remove the khanmigo theme in WB in favor of a separate DynamicActionButton component (future PR).
There was a problem hiding this comment.
Does this mean call sites that use a ModalPanel with a secondary Button will also needed to apply this work around?
There was a problem hiding this comment.
Not all call sites. This should be solved by WB internally as far as consumers use the light=false and closeButtonVisible prop. I'm planning to update the remaining cases in webapp.
There was a problem hiding this comment.
note: These variants are no longer accepted by PopoverContent as I removed the light variant from there a few weeks ago. I forgot to cleanup this bit.
| @@ -175,7 +160,8 @@ const ButtonCore: React.ForwardRefExoticComponent< | |||
| sharedStyles.endIconWrapper, | |||
| kind === "tertiary" && | |||
| sharedStyles.endIconWrapperTertiary, | |||
| (focused || hovered) && | |||
| !disabled && | |||
There was a problem hiding this comment.
note: Added this line as it is needed now that the complex css pseudo-selector is removed (e.g. :hover:not(aria...)).
| ":hover": hoverStyling, | ||
| ":focus-visible": focusStyling, | ||
| ":active": activePressedStyling, |
There was a problem hiding this comment.
note: This will simplify how selectors work (I took a similar approach with IconButton).
| @@ -34,5 +34,6 @@ export const inverse = { | |||
| borderRadius: border.radius.radius_080, | |||
| // This is a slightly darker color than the inverse color. | |||
| borderColor: pressColor, | |||
| background: `color-mix(in srgb, ${semanticColor.surface.primary} 5%, transparent)`, | |||
There was a problem hiding this comment.
note: This change is to provide a slightly darker bg color when pressed. You'll see it here:
https://www.chromatic.com/test?appId=5e1bf4b385e3fb0020b7073c&id=68013f858f1709244ee14785
This change was made so we control a bit more the contrast ratio in the press state.
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 (add1dc1) 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="PR2563"Packages can also be installed manually by running: pnpm add @khanacademy/wonder-blocks-<package-name>@PR2563 |
| // NOTE: This is a workaround while we move | ||
| // this button to DAB and remove the | ||
| // `Button.khanmigo` theme. | ||
| { | ||
| background: "transparent", | ||
| ":hover": {background: "transparent"}, | ||
| }, |
There was a problem hiding this comment.
Does this mean call sites that use a ModalPanel with a secondary Button will also needed to apply this work around?
| [":active:not([aria-disabled=true])" as any]: | ||
| activePressedStyling, | ||
| ":hover": hoverStyling, | ||
| ":focus-visible": focusStyling, |
There was a problem hiding this comment.
Will we move this to use the global focus styling later on? 😄
There was a problem hiding this comment.
That's correct, I didn't want to introduce too many visual changes in this PR, and I'm planning to cut a release with this change only for now. I checked in webapp and there are ~43 instances of Button.light, so I want to avoid having to include more changes than needed atm.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2563 +/- ##
============================
============================
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Summary:
This PR mainly removes the
lightprop from theButtoncomponent and itsassociated styles. The
lightprop was used to apply a light theme to thebutton, but it is no longer maintained and has been removed to simplify the
codebase.
The
inverseaction style can now be used as a replacement for these kind ofscenarhos.
Details:
lightprop as it is no longer maintained.:hover,:active), instead of using:hover:not("aria-disabled=true).actionStyles.inverseto account forButtoninstances.Issue: https://khanacademy.atlassian.net/browse/WB-1852
Test plan:
Navigate to the
Buttoncomponent in storybook and verify that thelightpropis no longer available.
Test the
inverseaction style in storybook to ensure it looks as expected withButtoninstances.