WB-1808: Button - Use CSS pseudo-classes for styling states (hover, focus, etc) #2404
Conversation
…s-visible). Keep some clickable states for programmatic focus and preserve active/pressed overrides.
🦋 Changeset detectedLatest commit: 3aee270 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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: +69 B (+0.07%) Total Size: 96.4 kB
ℹ️ View Unchanged
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2404 +/- ##
============================
============================
Continue to review full report in Codecov by Sentry.
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-ckrjpvvmqt.chromatic.com/ Chromatic results:
|
| @@ -27,26 +26,6 @@ import ComponentInfo from "../../.storybook/components/component-info"; | |||
| import ButtonArgTypes from "./button.argtypes"; | |||
| import {ThemeSwitcherContext} from "@khanacademy/wonder-blocks-theming"; | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
note: Got rid of this b/c it's duplicated. The source of truth is the code in button.tsx.
| chromatic: { | ||
| // We already have screenshots of other stories that cover more of | ||
| // the button states | ||
| disableSnapshot: true, | ||
| }, |
There was a problem hiding this comment.
note: I'm disabling a bunch of snapshots now that we can centralize visual regression tests in the All variants stories.
There was a problem hiding this comment.
note: Removed this to now use Chromatic snapshots instead of unit tests.
| ? buttonStyles.pressed | ||
| : focused && buttonStyles.focused), |
There was a problem hiding this comment.
note: Changed the style names to match what's returned from clickable-behavior.
| boxShadow: `0 0 0 1px ${boxShadowInnerColor}, 0 0 0 3px ${ | ||
| light ? theme.color.bg.primary.default : color | ||
| }`, |
There was a problem hiding this comment.
note: Took the opportunity to switch from boxShadow to outline. This could help solving this ticket: https://khanacademy.atlassian.net/browse/WB-1712
| textUnderlineOffset: theme.font.offset.default, | ||
| textDecoration: "underline", | ||
| textDecorationThickness: theme.size.underline.hover, |
There was a problem hiding this comment.
note: Also simplified the tertiary hover styles to rely on underlines instead of using the :after hack.
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 (99be186) 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="PR2404"Packages can also be installed manually by running: yarn add @khanacademy/wonder-blocks-<package-name>@PR2404 |
| // We already have screenshots of other stories that cover more of | ||
| // the button states | ||
| disableSnapshot: true, | ||
| }, | ||
| }; | ||
|
|
||
| export const Spinner: StoryComponentType = () => ( |
There was a problem hiding this comment.
Would it be helpful to include the spinner state in All Variants as well?
There was a problem hiding this comment.
I thought about that one, but I decided to keep it separately as it could introduce flakyness to that snapshot. I'm mentioning that because this variant includes animation and I've seen that Chromatic sometimes takes screenshots at different times.
That said, maybe we could add it and see how it goes? wdyt?
There was a problem hiding this comment.
Good point! I was thinking it could be helpful to see it across the different themes/states/variants etc, though I understand snapshots for animated things can be flaky! I'm okay leaving it as a separate story!
| @@ -25,8 +30,6 @@ const theme = { | |||
| primary: { | |||
| default: tokens.color.white, | |||
| disabled: tokens.color.offBlack32, | |||
| // used in boxShadow | |||
| inverse: tokens.color.darkBlue, | |||
There was a problem hiding this comment.
question: When we remove things from a theme, is it considered a breaking change? It seems like it isn't since we don't export the theme from the package explicitly, though thought I'd double check!
I was also wondering if we needed to remove this from other themes, but I see now that the khanmigo theme doesn't have these set since we merge the default theme with the khanmigo theme!
There was a problem hiding this comment.
Good question, and you are right. Right now, as theming works, this is only available internally, so it wouldn't be a breaking change.
The other themes are built based on the shape of the default object, and that means that if we remove a key that is used in a merged theme (e.g. khanmigo), then TS would complain about it :). I created that merge theme architecture having that in mind, so it would help us to have a more robust theming approach.
There was a problem hiding this comment.
Nice! Good to know this about our theming!
| outlineOffset: 2, | ||
| outlineStyle: "solid", | ||
| outlineWidth: 2, |
There was a problem hiding this comment.
(For my own learning): When do we include values in a theme vs setting the style directly?
There was a problem hiding this comment.
ohhhh thanks for being curious. This should be part of the theme as it could allow us in the future to configure these (e.g. in Polaris). I'm going to use a theme variable for this.
There was a problem hiding this comment.
@beaesguerra I've updated the styles to use theme now.
| const focusStyling = { | ||
| background: light | ||
| ? theme.color.bg.secondary.inverse | ||
| : theme.color.bg.secondary.focus, | ||
| borderColor: "transparent", | ||
| outlineColor: light ? theme.color.border.primary.inverse : color, | ||
| outlineStyle: "solid", | ||
| outlineWidth: theme.border.width.focused, | ||
| }; | ||
|
|
||
| const activePressedStyling = { | ||
| background: light ? activeColor : secondaryActiveColor, | ||
| color: light ? fadedColor : activeColor, | ||
| borderColor: "transparent", | ||
| outlineColor: light ? fadedColor : activeColor, | ||
| outlineStyle: "solid", | ||
| outlineWidth: theme.border.width.focused, | ||
| }; |
There was a problem hiding this comment.
It'll be so nice when we can share state styling across different components 😄
There was a problem hiding this comment.
Yess!!! 1000% to that 🙏
| [":hover:not([aria-disabled=true])" as any]: focusStyling, | ||
| [":focus-visible:not([aria-disabled=true])" as any]: | ||
| focusStyling, | ||
| [":active:not([aria-disabled=true])" as any]: |
There was a problem hiding this comment.
Is there something we can do to prevent having as any?
I was trying to see if I ran into similar things when working on the SelectOpener and it looks like we don't type cast it explicitly because we set newStyles in SelectOpener to Record<String, any> instead of Record<String, CSSProperties> 😅
There was a problem hiding this comment.
ugh yeah that's very annoying tbh. This is one of the things I'd hope Aphrodite would have better support for. We could add types, but it's hard to calculate all the possible combinations/variants for these selectors.
I guess this is a good/solid point for switching to CSS selectors (aka css-modules). Well, you know that I worry about opening the door to specificity, but that's something that we'll have to evaluate when we make the definitive decision to switch our styling approach.
There was a problem hiding this comment.
There was a problem hiding this comment.
Oooh cool! That TS example looks promising! Looks like the ordering would matter. For example, :not([aria-disabled]):hover wouldn't meet the type requirements like :hover:not([aria-disabled])
There was a problem hiding this comment.
🎉 Thanks for removing snapshot tests now that we have Chromatic coverage!
|
Tested in Chrome, Safari, and Firefox on Mac, and Chrome and Firefox on Windows. The interaction states work great! Nice work @jandrade! |
Summary:
Changing the way we style different states. We are going to rely on CSS
pseudo-classes for styling states (
:hover,:focus-visible).Note that we have to keep some
ClickableBehaviorstates for programmatic focusand preserve active/pressed overrides.
Also took the opportunity to modify the primary variant to use
outline + outlineOffsetinstead ofboxShadow.Issue: https://khanacademy.atlassian.net/browse/WB-1808
Test plan:
Navigate to
/?path=/docs/packages-button--docsand verify that the buttons arestyled correctly.