-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tabs: render tabs as Button
components by default
#57121
Conversation
Flaky tests detected in ff2f5f0. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7226258203
|
Button
components by defaultButton
components by default
ff2f5f0
to
5db8f6c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code-wise, the changes look good.
cc'ing @mirka around the topic of using Button
instead of a vanilla HTML button
in Tabs
— if text alignment and hover color are the only things we are worried about, we could also consider adding those styles to Tabs
without using Button
at all.
Also cc @WordPress/gutenberg-design for awareness
Let's see if I am in the right place: Tab 3. Not sure what else to do here. I updated the Gutenberg plugin and compared for instance Background color tabs in relation to Storybook. A sidetrack. |
The difference in text alignment is noticeable only when the Tab is wider than its text contents — for example, in Storybook, we could manually add a
I encourage you to discuss this in a separate issue, as the |
Yes, I'm usually apprehensive about using a styled component like |
Cool, that was my angle too. Let's stick with |
Thank you both for the input! Closing this PR in favor of #57275, which will add the necessary styles to the |
What?
Modifies the
Tabs.Tab
subcomponent to render as aButton
component instead of a plainbutton
html element.Why?
In an effort to avoid making the
Tabs
component overly opinionated, we opted for abutton
element and an optionalrender
prop, so consumers could use aButton
if they wanted one for things like icons and tooltips.At the time everything with this approach seemed good, but I've since noticed that there are some style differences that I'd missed until now. Specifically:
Button
adds an accent color to the text of tertiary buttons when hovered. Thebutton
element rendered byTabs
viaAriakit
does not do this.display: inline-flex
and subsequentalign-items: center
provided by theButton
component's styles.How?
A performing a
nullish
check when passing therender
prop down toAriakit
, we can render whatever the consumer wants (including a more detailedButton
with an icon and tooltip), and fall back to a standardButton
component if norender
prop is provided. All of the props passed toTabs.Tab
will flow through therender
prop to the defaultButton
component.Testing Instructions
To begin, apply this diff. It increases the width of the tabs on the default story template
Testing Instructions
Tabs
stories. Confirm that the tabs are properly highlighted when hovered.