Tabs: Styling #2545
Tabs: Styling #2545
Conversation
… subcomponent in the docs
…animation so it can be used by both tabs and navigation-tabs
🦋 Changeset detectedLatest commit: e6cb053 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: +643 B (+0.62%) Total Size: 105 kB
ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-onhdyxknjc.chromatic.com/ Chromatic results:
|
| @@ -0,0 +1,141 @@ | |||
| import { | |||
There was a problem hiding this comment.
Most of this is the logic that was in NavigationTabs. Will point out things that are different to support both types of tabs
|
|
||
| // Find the active tab | ||
| const activeTab = Array.from(tabsContainerRef.current.children).find( | ||
| isTabActive, |
There was a problem hiding this comment.
We make isTabActive a prop since we use aria-current in NavigationTabs and aria-selected in Tabs to determine if a tab is active.
| @@ -2,12 +2,12 @@ import { | |||
| addStyle, | |||
| AriaProps, | |||
| StyleType, | |||
| useOnMountEffect, | |||
There was a problem hiding this comment.
Pulling out the logic for the animated selected tab indicator so it can be shared with Tabs.
I was starting to share the styles between Tabs and NavigationTabs, but found that the way the styles are applied is a bit different since their html structures are different.
NavigationTabs has extra nesting due to the ul and li elements:
<nav>
<ul>
<li>
<a href="#">
Link
</a>
</li>
<li>
<a href="#">
Link
</a>
</li>
</ul>
</nav>
Tabs:
<div>
<div role="tablist>
<button role="tab" />
<button role="tab" />
</div>
<div role="tabpanel"/>
<div role="tabpanel" />
</div>
| [":after" as any]: { | ||
| height: border.width.hairline, | ||
| backgroundColor: | ||
| semanticColor.action.secondary.progressive.hover.foreground, |
There was a problem hiding this comment.
Using :after styling to apply the hover/pressed underline styling instead of box-shadow because we use box-shadow for the focus outline.
In NavigationTabs, the Link and NavigationTabItem components were used to represent a tab, so we were able to use box-shadow for the hover/pressed underline styling since it was on the NavigationTabItem component instead of the Link (which has the focus styling)
There was a problem hiding this comment.
This could be helpful as a summarized comment in the code.
FWIW I think it would be fine using box-shadow directly as we use a combination of box-shadow + outine for the global focus online. So using this approach would be only overriding the inner white ring, but the outer blue would remain there.
Do you think it would make sense to keep it this way?
| </> | ||
| ), | ||
| globals: { | ||
| zoom: "400%", |
There was a problem hiding this comment.
This is working as expected in Chromatic now! I need to follow up with the Chromatic support team still on the other flaky tests we were seeing before
| return tabElement.ariaSelected === "true"; | ||
| }, []); | ||
|
|
||
| const {indicatorProps, updateUnderlineStyle} = useTabIndicator({ |
There was a problem hiding this comment.
This uses the same logic as NavigationTabs for the selected tab indicator animation.
I created https://khanacademy.atlassian.net/browse/WB-1924 for adding animations to the tab panel. This is something that won't be needed for NavigationTabs since that relies on link navigation.
beaesguerra
force-pushed
the
wb-1908-tabs-styles
branch
from
ff93caf to
08039a9
Compare
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 (3b49b81) 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="PR2545"Packages can also be installed manually by running: pnpm add @khanacademy/wonder-blocks-<package-name>@PR2545 |
beaesguerra
force-pushed
the
wb-1908-tabs-styles
branch
from
7ca962c to
e6cb053
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feature/tabs #2545 +/- ##
====================================
====================================
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
| // TODO: Update to use spacing tokens | ||
| gap: "4px", |
There was a problem hiding this comment.
suggestion: I guess this is possible now that Marcy has landed the new spacing tokens :).
| <> | ||
| <AllVariants rows={rows} columns={columns}> | ||
| {(props) => ( | ||
| <View> |
There was a problem hiding this comment.
suggestion: This View could be removed if it is not necessary.
| export default { | ||
| title: "Packages / Tabs / Tabs / Tabs", | ||
| title: "Packages / Tabs / Tabs", |
| component: Tabs, | ||
| subcomponents: {Tab}, |
There was a problem hiding this comment.
praise: Ohh this makes total sense! nice add.
| export const Scenarios: StoryComponentType = { | ||
| render: (args) => { | ||
| return ( | ||
| <ScenariosLayout scenarios={scenarios}> | ||
| {(props) => <ControlledTabs {...args} {...props} />} | ||
| </ScenariosLayout> | ||
| ); | ||
| }, | ||
| args: { | ||
| animated: true, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
thought: (no change required).... this is really useful for testing/internal purposes. How do you feel about creating a separate stories file to cover this? I'm not sure if we should include this as part of the Docs page, or if it is still useful for devs to understand/test. wdyt?
| const isTabActive = React.useCallback( | ||
| (navTabItemElement: Element): boolean => { | ||
| // Check the link inside the NavigationTabItem to see if it has aria-current="page" | ||
| return navTabItemElement.children[0]?.ariaCurrent === "page"; |
There was a problem hiding this comment.
question: Unrelated to this PR..... Would there be a case where the first element is NOT a Link? I don't think so, but want to confirm if this could be an edge case.
| [":after" as any]: { | ||
| height: border.width.hairline, | ||
| backgroundColor: | ||
| semanticColor.action.secondary.progressive.hover.foreground, |
There was a problem hiding this comment.
This could be helpful as a summarized comment in the code.
FWIW I think it would be fine using box-shadow directly as we use a combination of box-shadow + outine for the global focus online. So using this approach would be only overriding the inner white ring, but the outer blue would remain there.
Do you think it would make sense to keep it this way?
Summary:
stylespropanimatedpropIssue: WB-1908
Test plan:
?path=/story/packages-tabs-tabs--scenarios)?path=/story/packages-tabs-tabs-subcomponents-tab-all-variants--default)?path=/story/packages-tabs-tabs-tabs-all-variants--default)animatedprop works as expected (?path=/story/packages-tabs-tabs--animated)stylesprop works as expected (?path=/story/packages-tabs-tabs--custom-styles)Implementation Plan