NavigationTabs Styling #2508
NavigationTabs Styling #2508
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
🦋 Changeset detectedLatest commit: 9c4fa3d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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: +299 B (+0.3%) Total Size: 100 kB
ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-faxousjuul.chromatic.com/ Chromatic results:
|
…ndle different scenarios
| const withZoom: Decorator = (Story, context) => { | ||
| if (context.globals.zoom) { | ||
| return ( | ||
| <div style={{ zoom: context.globals.zoom }}> |
There was a problem hiding this comment.
I was trying to find a way for us to easily configure a story to be a specific zoom level so we can have some Chromatic tests covering those cases too. I came across the CSS zoom property to control magnification
.storybook/preview.tsx
Outdated
| direction: { | ||
| description: "The language direction to use", | ||
| toolbar: { | ||
| title: "Language Direction", | ||
| icon: "globe", | ||
| items: [ | ||
| { | ||
| value: "ltr", | ||
| icon: "arrowrightalt", | ||
| title: "Left to Right", | ||
| }, | ||
| { | ||
| value: "rtl", | ||
| icon: "arrowleftalt", | ||
| title: "Right to Left", | ||
| }, | ||
| ], | ||
| dynamicTitle: true, | ||
| }, | ||
| }, | ||
| zoom: { | ||
| description: "Preset zoom level", | ||
| toolbar: { | ||
| title: "Zoom Presets", | ||
| icon: "zoom", | ||
| items: [ | ||
| { | ||
| // undefined so the there is no zoom value set | ||
| value: undefined, | ||
| title: "default", | ||
| }, | ||
| { | ||
| value: "2", | ||
| title: "200%", | ||
| }, | ||
| { | ||
| value: "4", | ||
| title: "400%", | ||
| }, | ||
| ], | ||
| }, | ||
| } |
There was a problem hiding this comment.
There are 2 new controls in our toolbar! We can also configure these global parameters for specific stories
| </View> | ||
| )} | ||
| </AllVariants> | ||
| <div dir="rtl"> |
There was a problem hiding this comment.
Including the rtl block as part of the same all variants story so that it can be covered in the different states (hover, focus, etc)
There was a problem hiding this comment.
Nice! I did something similar with Link here:
wonder-blocks/__docs__/wonder-blocks-link/link-variants.stories.tsx
Lines 56 to 90 in b6d77dc
There was a problem hiding this comment.
For later.... it would be nice defining a general pattern in all-variants.tsx to include RTL by default, but we can define that later if needed.
There was a problem hiding this comment.
Haha yes I was referencing your work for the AllVariants with rtl!
For later.... it would be nice defining a general pattern in all-variants.tsx to include RTL by default, but we can define that later if needed.
While working on this, I attempted to implement this because I thought it'd be nice to include the rtl cases by default! I then realized I had to change the labels for rtl so it wasn't using English. Thinking back, this probably could be solved by passing a boolean to the children function for whether or not it is rtl. Here's the commit I tried it in 4d233ce .
On a related note, I was also wondering if we wanted the AllVariants component to cover the hover, focus, press states too in one story instead having one story for each state combination. We could do this by wrapping each table with an id, and then specifying specific selectors when we configure the pseudo states add on:
parameters: {
pseudo: {
hover: [
`#${stateElementIds.hover} *`,
`#${stateElementIds.hoverAndFocus} *`,
],
focusVisible: [
`#${stateElementIds.focus} *`,
`#${stateElementIds.hoverAndFocus} *`,
],
active: [`#${stateElementIds.press} *`],
},
},
While it's possible, I'm not sure if it would be helpful to include it all in one sticker sheet. It would save snapshot amounts though since we'd have ~5 snapshots per component due to the different states!
Curious to hear if anyone has thoughts on this! Happy to iterate on this in another PR :)
There was a problem hiding this comment.
oh merging pseudo states would be worth experimenting, and then we could see if there's benefit from that change. how does that sound?
There was a problem hiding this comment.
I experimented with merging the pseudo states and rtl in the same story: #2511
Left some comments in the draft PR too, feel free to provide feedback in that PR!
| </div> | ||
| ), | ||
| globals: { | ||
| zoom: "400%", |
There was a problem hiding this comment.
Here's an example of how we can configure the new zoom decorator!
beaesguerra
force-pushed
the
nav-tabs-style
branch
3 times, most recently
from
897067d to
a09e8f5
Compare
beaesguerra
force-pushed
the
nav-tabs-style
branch
from
a09e8f5 to
0cdf1bd
Compare
| parameters: { | ||
| chromatic: { | ||
| // Disabling because Chromatic crops the story when zoom is used | ||
| disableSnapshot: true, |
There was a problem hiding this comment.
I disabled the Zoom stories in Chromatic because the stories would get cropped in the snapshots 😢 here is an example in Chromatic. I tried some things to address this but couldn't get it working. Let me know if you have other ideas!
- setting the
viewportconfig for Chromatic for that story - adding a delay for the snapshot
- setting width: 100% on the decorator
There was a problem hiding this comment.
Weird. Maybe try min-width: 100vw, and/or some borders to see if you can determine which top-level element is being cut off?
There was a problem hiding this comment.
have you tried using viewports?
There was a problem hiding this comment.
Thanks for the suggestions!
- Setting
min-width: 100vwmade it so the NavigationTabs are no longer horizontally scrollable since it allows the container to expand. We want to be able to observe the scrolling behaviour! - Related to that, I tried setting
width: 100vwandmaxWidth: 100vw. The snapshot shows more of the contents, though we lose the horizontal scrolling in the NavigationTabs (it's put on the story instead). Chromatic snapshot - Setting
viewportsdidn't work. Chromatic with viewports config - Setting
defaultViewportdidn't work too (the snapshot was less wide too for some reason) Chromatic with defaultViewport
I went back to disabling the Zoom stories 😅 I think at least the stories are functional and show the expected behaviour when you try it out in Storybook! One other idea is we could reach out to the Chromatic team to see if they've seen this issue before. (I think it's a Chromatic issue since the story renders as expected and it's only the snapshot that is cropped!). @jandrade how do you normally contact Chromatic for support?
| const styles = StyleSheet.create({ | ||
| root: { | ||
| listStyle: "none", | ||
| display: "inline-flex", | ||
| [":has(a:hover)" as any]: { | ||
| boxShadow: `inset 0 -${sizing.size_050} 0 0 ${semanticColor.action.secondary.progressive.hover.foreground}`, |
There was a problem hiding this comment.
I mostly used the secondary action semantic tokens. We may need to revisit this though once Thunder Blocks theming is in place. (related Figma thread)
There was a problem hiding this comment.
question: Do you know if this is the right box-shadow width? I was looking at the design specs, and it looks like the underline width for hover is 2px (size_025).
There was a problem hiding this comment.
Good catch!! Updated the hover underline to be 2px tall. I also made sure the current + hover state remains the same at 4px!
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 (cac8b31) 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="PR2508"Packages can also be installed manually by running: pnpm add @khanacademy/wonder-blocks-<package-name>@PR2508 |
There was a problem hiding this comment.
Great work @beaesguerra! I left you a few really tiny suggestions. Excited to see this component in the wild!
| }; | ||
|
|
||
| /** | ||
| * The following story shows how the component habdles specific scenarios. |
There was a problem hiding this comment.
nit: tiny typo!
| * The following story shows how the component habdles specific scenarios. | |
| * The following story shows how the component handles specific scenarios. |
.storybook/preview.tsx
Outdated
| icon: "zoom", | ||
| items: [ | ||
| { | ||
| // undefined so the there is no zoom value set |
There was a problem hiding this comment.
nit: extra word?
| // undefined so the there is no zoom value set | |
| // undefined so there is no zoom value set |
| ...row.props, | ||
| ...col.props, | ||
| }, | ||
| `${row.name} ${col.name}`, |
There was a problem hiding this comment.
Oh interesting, I just did something super similar in #2509!
There was a problem hiding this comment.
Nice!! I was also addressing a11y warnings in the new stories! I see in #2509 we add aria-label to the props. I ended up adding a new argument for the name so it could be used for a unique aria-label. I was thinking not all components may support an aria-label at the root level so providing it as an arg keeps it flexible! What do you think?
| </div> | ||
| ), | ||
| globals: { | ||
| zoom: "400%", |
| parameters: { | ||
| chromatic: { | ||
| // Disabling because Chromatic crops the story when zoom is used | ||
| disableSnapshot: true, |
There was a problem hiding this comment.
Weird. Maybe try min-width: 100vw, and/or some borders to see if you can determine which top-level element is being cut off?
| a11y: { | ||
| config: { | ||
| rules: [ | ||
| // Disabling warning: "Element's background color could not |
There was a problem hiding this comment.
I'm curious about this one, as I wrote part of the overlapping logic in the color-contrast rule! It looks like the Axe extension doesn't surface incompletes anymore, and I don't see any Needs Review items in Accessibility Insights either. Maybe Storybook is using an older axe-core API version?
There was a problem hiding this comment.
It was showing as an incomplete in the Storybook add on! Any tips on how to address this?
I also see it in the labels for the story! Might be related to setting the CSS zoom property?
| alt="Wonder Blocks logo" | ||
| /> | ||
| <SingleSelect | ||
| placeholder="Placeholder" |
There was a problem hiding this comment.
This one needs a label with the new API!
| placeholder="Placeholder" | |
| aria-label="Dropdown" | |
| placeholder="Placeholder" |
| </View> | ||
| )} | ||
| </AllVariants> | ||
| <div dir="rtl"> |
There was a problem hiding this comment.
Nice! I did something similar with Link here:
wonder-blocks/__docs__/wonder-blocks-link/link-variants.stories.tsx
Lines 56 to 90 in b6d77dc
| </View> | ||
| )} | ||
| </AllVariants> | ||
| <div dir="rtl"> |
There was a problem hiding this comment.
For later.... it would be nice defining a general pattern in all-variants.tsx to include RTL by default, but we can define that later if needed.
__docs__/components/all-variants.tsx
Outdated
| <div | ||
| style={{ | ||
| padding: sizing.size_100, | ||
| marginBlock: sizing.size_100, | ||
| border: `${border.width.hairline}px dashed ${semanticColor.border.primary}`, | ||
| }} | ||
| > |
There was a problem hiding this comment.
suggestion: It would be great switching to View (or StyledDiv if view doesn't fit) to prevent inlining the CSS in the html.
| <div | |
| style={{ | |
| padding: sizing.size_100, | |
| marginBlock: sizing.size_100, | |
| border: `${border.width.hairline}px dashed ${semanticColor.border.primary}`, | |
| }} | |
| > | |
| <View | |
| style={{ | |
| padding: sizing.size_100, | |
| marginBlock: sizing.size_100, | |
| border: `${border.width.hairline}px dashed ${semanticColor.border.primary}`, | |
| }} | |
| > |
Expanding on this, the styles could be moved to the styles object as well.
There was a problem hiding this comment.
Thanks for the reminder! Updated :)
| </View> | ||
| )} | ||
| </AllVariants> | ||
| <div> |
There was a problem hiding this comment.
suggestion: You could also use a fragment to avoid adding extra divs.
| <div> | |
| <> |
| </View> | ||
| )} | ||
| </AllVariants> | ||
| <div> |
| @@ -141,6 +227,58 @@ export const Active: Story = { | |||
| parameters: {pseudo: {hover: true, active: true}}, | |||
| }; | |||
|
|
|||
| export const Zoom: Story = { | |||
| render: (args) => ( | |||
| <div> | |||
| color: semanticColor.action.secondary.progressive.default | ||
| .foreground, | ||
| outline: "none", | ||
| boxShadow: `0 0 0 ${sizing.size_025} ${semanticColor.focus.inner}, 0 0 0 ${sizing.size_050} ${semanticColor.focus.outer}`, |
There was a problem hiding this comment.
praise: Yay!! it is great finally seeing both semantic focus tokens in place 🎉
| const styles = StyleSheet.create({ | ||
| root: { | ||
| listStyle: "none", | ||
| display: "inline-flex", | ||
| [":has(a:hover)" as any]: { | ||
| boxShadow: `inset 0 -${sizing.size_050} 0 0 ${semanticColor.action.secondary.progressive.hover.foreground}`, |
There was a problem hiding this comment.
question: Do you know if this is the right box-shadow width? I was looking at the design specs, and it looks like the underline width for hover is 2px (size_025).
| root?: StyleType; | ||
| list?: StyleType; |
There was a problem hiding this comment.
thought: What do you think about extending the description a bit to specify what root and list are specifically for?
…rent + hover doesn't have a visual difference
Summary:
The following changes address various styling requirements and includes adding some tools for testing in Storybook
Props
stylesprop and NavigationTabs supportsstyleprop for custom stylingStyles
<li>element instead of the<a>Storybook
text-for-testing.ts)Issue: WB-1898
Test plan: