Experimenting with single AllVariants story for all pseudo states #2511
Conversation
…er. Since it's put on the li, it won't conflict with the focus outline box shadow later on
This reverts commit 4d233ce.
…ead at smaller screen sizes
…p out the label based on rtl
…use we swap out the rows since the children props for NavTabItem changes [all-variants-update] cleanup
|
… is because we swap out the rows since the children props for NavTabItem changes" This reverts commit 9e0f300.
|
Size Change: 0 B Total Size: 100 kB ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-gcfobwydcu.chromatic.com/ Chromatic results:
|
| /** | ||
| * The states to render. | ||
| */ | ||
| states?: State[]; |
There was a problem hiding this comment.
Making it optional to configure the states to support in case there are other states to configure outside of base, hover, focus, press, and hover+focus
| * The children as a function that receives the state props used to render | ||
| * each variant of the component. | ||
| */ | ||
| children: (props: any, name: string, isRtl?: boolean) => React.ReactNode; |
There was a problem hiding this comment.
Same as children prop from AllVariants, except we include isRtl to we could choose to toggle labels for the children
| @@ -93,3 +98,20 @@ const styles = StyleSheet.create({ | |||
| gap: spacing.medium_16, | |||
| }, | |||
| }); | |||
|
|
|||
| export const StickerSheet: Story = { | |||
There was a problem hiding this comment.
This shows a straightforward example of using AllVariantsStates and changing the label based on the isRtl arg
| { | ||
| name: "Default", | ||
| props: { | ||
| children: (label: string) => <Link href="#link">{label}</Link>, |
There was a problem hiding this comment.
Refactored the rows so that the label for the link is flexible for the cells. We make children a function instead so we can pass in the label based on if it's in rtl mode
| // showRtl=false because we have a separate story for rtl with rtlRows | ||
| <AllVariantsStates rows={rows} columns={columns} showRtl={false}> |
There was a problem hiding this comment.
NavigationTabs is a bit different in that the variety in the examples is based on the props of the children components, rather than the props of NavigationTabs. It also renders multiple NavigationTabItems as children.
To simplify things, we have rows and rtlRows, which has rtl labels instead. Instead of letting AllVariantsStates render the rtl cells, we create a separate story with the rtlRows instead. We can also use the global direction to configure it to be in rtl mode.
Let me know if you have any ideas on how to address this! Or if you have thoughts on this in general.
There was a problem hiding this comment.
This is a super interesting experiment! thanks for looking into this 🫶
Do we want to combine all the pseudo states into one story?
Is it managable to review visual changes in Chromatic with these larger stories?
I'm seeing that in some cases there's a lot of information in the snapshot, so I'm not sure if that would make it harder to review and/or find small changes. How do you feel about that?
Do we want to include rtl for all the pseudo states?
IMO yes! we should include rtl as a first-class citizen in our visual regression tests.
What about the approach for zoom?
Where you able to fix the viewport issue?
| type State = {name: string; id: string}; | ||
|
|
||
| export const commonStates = { | ||
| base: {name: "Base", id: "base"}, |
There was a problem hiding this comment.
thought: How do you feel about changing this to Rest to be consistent on how Design uses these states in Figma? Would love to hear your thoughts on why base could apply better here (if that's the case).
There was a problem hiding this comment.
Rest makes sense! Will update :)
| export const defaultPseudoStates = { | ||
| hover: [ | ||
| `#${commonStates.hover.id} *`, | ||
| `#${commonStates.hoverAndFocus.id} *`, | ||
| ], | ||
| focusVisible: [ | ||
| `#${commonStates.focus.id} *`, | ||
| `#${commonStates.hoverAndFocus.id} *`, | ||
| ], | ||
| active: [`#${commonStates.press.id} *`], | ||
| }; |
There was a problem hiding this comment.
praise: Ohh this is neat! Very smart solution :). this is using this feature right? https://github.com/chromaui/storybook-addon-pseudo-states?tab=readme-ov-file#targeting-specific-elements
There was a problem hiding this comment.
Yes! Then we can target specific sections with the selector styles 😄
I find when there are many small changes in the same snapshot, it takes longer for me to review all the details (that might just be because we're making more changes right now to support polaris!). With that said, this set up with all the states doesn't account for different themes yet! Would we want it to? Or are we still exploring using Thinking about themes, it might be easier to see all the states for one theme in one snapshot. So each component would have a stickersheet for each theme. Let me know what you think! If we want to combine the states into one story, I'll clean up this branch and open a separate PR for it!
I reached out to Chromatic and they suggested adding a delay. That didn't fix things so they escalated it to their engineers and will follow up! |
…es in the same snapshot (#2550) ## Summary: Builds based on the experiment introduced in #2511 to add a `StateSheet` component to Storybook. This component will be used to test snapshots similar to the Figma Handshake file. The StateSheet component will allow us to create a visual representation of the different states of our components, making it easier to test them while taking less snapshots. This change will also allow us to use this in combination of Chromatic modes in the near future to test combinations like `responsive`, `theming`, `rtl`, etc. Issue: "none" ## Test plan: Verify that the `IconButton - All variants` chromatic snapshot looks similar to the Handshake file. <img width="1354" alt="Screenshot 2025-04-11 at 12 39 37 PM" src="https://github.com/user-attachments/assets/74997401-34d3-4f4e-92e3-d0d492f6500b" /> Author: jandrade Reviewers: beaesguerra, jandrade Required Reviewers: Approved By: beaesguerra Checks: ✅ 10 checks were successful, ⌛ 1 check is pending, ⏭️ 2 checks have been skipped Pull Request URL: #2550
|
Closing since this has been iterated on in #2550 ! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2511 +/- ##
============================
============================
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Summary:
Explore how we could combine different variants, states, rtl, and zoom altogether.
Context: #2508 (comment)
Looking for feedback around:
Issue: WB-XXXX
Test plan: