Directory

Tabs: render tabs as `Button` components by default by chad1008 · Pull Request #57121 · WordPress/gutenberg · GitHub
Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Tabs: render tabs as Button components by default #57121

Closed
wants to merge 3 commits into from
Closed

Tabs: render tabs as Button components by default #57121

wants to merge 3 commits into from

Conversation

chad1008
Copy link
Contributor

@chad1008 chad1008 commented Dec 15, 2023 •
edited

What?

Modifies the Tabs.Tab subcomponent to render as a Button component instead of a plain button html element.

Why?

In an effort to avoid making the Tabs component overly opinionated, we opted for a button element and an optional render prop, so consumers could use a Button 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:

  1. Hover effect: Button adds an accent color to the text of tertiary buttons when hovered. The button element rendered by Tabs via Ariakit does not do this.
  2. Text alignment: This one is only apparent in cases where the tabs width is expanded, as it is in the List View implementation. In cases like this, where the button is wider than the text it contains, the text becomes centered, rather than left aligned. This is because it lacks the display: inline-flex and subsequent align-items: center provided by the Button component's styles.

How?

A performing a nullish check when passing the render prop down to Ariakit, we can render whatever the consumer wants (including a more detailed Button with an icon and tooltip), and fall back to a standard Button component if no render prop is provided. All of the props passed to Tabs.Tab will flow through the render prop to the default Button component.

Testing Instructions

To begin, apply this diff. It increases the width of the tabs on the default story template 
diff --git a/packages/components/src/tabs/stories/index.story.tsx b/packages/components/src/tabs/stories/index.story.tsx
index 0e7ab725e3..f344767668 100644
--- a/packages/components/src/tabs/stories/index.story.tsx
+++ b/packages/components/src/tabs/stories/index.story.tsx
@@ -40,9 +40,15 @@ const Template: StoryFn< typeof Tabs > = ( props ) => {
 	return (
 		<Tabs { ...props }>
 			<Tabs.TabList>
-				<Tabs.Tab tabId="tab1">Tab 1</Tabs.Tab>
-				<Tabs.Tab tabId="tab2">Tab 2</Tabs.Tab>
-				<Tabs.Tab tabId="tab3">Tab 3</Tabs.Tab>
+				<Tabs.Tab tabId="tab1" style={ { width: '33%' } }>
+					Tab 1
+				</Tabs.Tab>
+				<Tabs.Tab tabId="tab2" style={ { width: '33%' } }>
+					Tab 2
+				</Tabs.Tab>
+				<Tabs.Tab tabId="tab3" style={ { width: '33%' } }>
+					Tab 3
+				</Tabs.Tab>
 			</Tabs.TabList>
 			<Tabs.TabPanel tabId="tab1">
 				<p>Selected tab: Tab 1</p>

Testing Instructions

  1. Launch Storybook
  2. Experiment with the Tabs stories. Confirm that the tabs are properly highlighted when hovered.
  3. On the Default story, which has now has wider tabs, confirm that the text is left-aligned.
  4. Compared to trunk, confirm there aren't any other visual changes in Storybook
  5. Compared to trunk, confirm these styles are now present in the editor (ie. Editor Settings Sidebar, Block Inserter, and in the background color picker for blocks) but there aren't any other unexpected changes.

@chad1008 chad1008 added [Type] Bug An existing feature does not function as intended [Package] Components /packages/components labels Dec 15, 2023
@chad1008 chad1008 requested a review from ciampo December 15, 2023 18:04
@chad1008 chad1008 self-assigned this Dec 15, 2023
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 15, 2023 •
edited

Flaky tests detected in ff2f5f0.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7226258203
📝 Reported issues:

@chad1008 chad1008 mentioned this pull request Dec 15, 2023
Merged
@chad1008 chad1008 changed the title Tabs: render tabs Button components by default Tabs: render tabs as Button components by default Dec 15, 2023
ciampo
ciampo reviewed Dec 19, 2023
View reviewed changes
Copy link
Contributor

@ciampo ciampo left a 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 buttonin 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

@ciampo ciampo requested a review from a team December 19, 2023 15:18
@paaljoachim
Copy link
Contributor

Let's see if I am in the right place:
https://wordpress.github.io/gutenberg/?path=/docs/components-experimental-tabs--docs

Tab 3.
Text is left aligned.

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.
Can we begin the work of setting up a Tabs block? So the user can add tabs to the canvas/document.
Using the Tabs component and various settings similar to other blocks.

@ciampo
Copy link
Contributor

ciampo commented Dec 20, 2023

Let's see if I am in the right place: wordpress.github.io/gutenberg/?path=/docs/components-experimental-tabs--docs

Tab 3. Text is left aligned.

Not sure what else to do here. I updated the Gutenberg plugin and compared for instance Background color tabs in relation to Storybook.

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 flex: 1 to all Tabs to force then to fill the full width of the screen, which would show how the text is centered:

Screenshot 2023-12-20 at 09 28 40

A sidetrack.
Can we begin the work of setting up a Tabs block? So the user can add tabs to the canvas/document.
Using the Tabs component and various settings similar to other blocks.

I encourage you to discuss this in a separate issue, as the @wordpress/components package and the Tabs component are not mean to be used in editor blocks.

@mirka
Copy link
Member

mirka commented Dec 20, 2023

cc'ing @mirka around the topic of using Button instead of a vanilla HTML buttonin 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.

Yes, I'm usually apprehensive about using a styled component like Button for something that calls for substantially different styling. Any style breakage that could occur from upstream changes are our own responsibility, so it'd be on the Tabs component to have sufficient vizreg tests to catch them. In this case here it sounds like it'd be cheaper to add the required styles without using Button.

@ciampo
Copy link
Contributor

ciampo commented Dec 20, 2023

In this case here it sounds like it'd be cheaper to add the required styles without using Button.

Cool, that was my angle too. Let's stick with button and only add necessary styles.

@chad1008 chad1008 mentioned this pull request Dec 20, 2023
Merged
@chad1008
Copy link
Contributor Author

Thank you both for the input! Closing this PR in favor of #57275, which will add the necessary styles to the button 😄

@chad1008 chad1008 closed this Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ciampo ciampo ciampo left review comments

@ajitbohra ajitbohra Awaiting requested review from ajitbohra ajitbohra is a code owner

Assignees

@chad1008 chad1008

Labels
[Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@chad1008 @paaljoachim @ciampo @mirka