TabPanel: Add prop to allow disabling of a tab button by joshuatf · Pull Request #46471 · WordPress/gutenberg

joshuatf · 2022-12-12T18:23:24Z

What?

Adds a prop to allow disabling of a given tab.

Why?

In WooCommerce we plan to disable tabs depending on a product configuration. While we could hide the disabled tabs from view to solve this, it's helpful for users to see disabled tabs and hover them for hints as to why certain tabs are disabled.

How?

Adds a prop to disable the tab. Alternatively we could pass ...rest to the tab button to allow passing of any props, but I'm not sure if this is wanted and would not have the advantages of TS.

Testing Instructions

Run npm run test:unit tab-panel and make sure all tests pass
Optionally create a tab panel with a a disabled prop

			<TabPanel
				tabs={ [
					{
						name: 'delta',
						title: 'Delta',
						className: 'delta-class',
						disabled: true,
					},
				] }
				children={ () => undefined }
				onSelect={ () => undefined }
			/>

Make sure the tab is disabled

Testing Instructions for Keyboard

Screenshots or screencast

alexstine

I think code looks good. I just need a little more time to A11Y check this. Passing disabled to Button component is okay since we are handling that for screen readers, but not sure how it behaves down below in NavigableMenu.

mirka

A few details to work out, but this looks good to me as well 👍 Thanks!

Two more things from my end:

We need to add a changelog before merging.
I think it would be helpful to add a Storybook story for this disabled use case, since it's a bit hard to discover at the moment. It would also be easier to catch styling bugs moving forward.

packages/components/src/tab-panel/types.ts

packages/components/src/tab-panel/test/index.tsx

packages/components/src/tab-panel/index.tsx

joshuatf · 2022-12-14T20:33:54Z

Thanks for the feedback @alexstine and @mirka. I added some additional logic for updating selection when tabs are disabled as well as additional tests around this logic.

This is now visible in Storybook which demonstrates the initial tab selection, but you can also verify that selection updates to the first enabled tab if the current tab is disabled by editing the raw tabs value in Storybook. E.g., select "Tab 3" and then update the prop data to:

[
  {
    "name": "tab1",
    "title": "Tab 1"
  },
  {
    "name": "tab2",
    "title": "Tab 2"
  },
  {
    "name": "tab3",
    "title": "Tab 3",
    "disabled": true
  }
]

Please let me know if everything looks good now- thanks!

joshuatf · 2022-12-19T22:21:06Z

Hi @mirka @alexstine - could you give this PR another look when you have time? Thanks!

mirka

Beautiful, thank you ✨ Code is clean, good fallback logic, and good test coverage. Accessibility looks good to me as well — markup and keyboard nav works as expected.

I think we're good to merge, unless @alexstine has any other concerns or would like to block it until he has some time to test.

alexstine

@joshuatf Sorry for the delay, I have been a little under the weather lately.

Looks good.

mirka · 2022-12-23T11:08:50Z

Almost forgot, let's update the component readme before we merge.

joshuatf · 2022-12-29T21:37:22Z

Thank you both for the reviews, @alexstine and @mirka! Updated the readme in b991862.

ciampo · 2023-01-02T14:48:45Z

packages/components/src/tab-panel/index.tsx

+			);
 		}
-	}, [ tabs, selectedTab?.name, initialTabName, handleTabSelection ] );
+		if ( selectedTab?.disabled && firstEnabledTab ) {


Nitty, but — right now, the code makes it look like the handleTabSelection function can be called twice during the same effect.

Should we rewrite the logic slightly (e.g. else if) to make it clear that these two code branches should be exclusive?

Sure! Done in c64730c.

ciampo

Thank you for working on this! 🎉

scruffian · 2023-01-11T17:23:16Z

I think this might have caused this issue: #47079

talldan · 2023-01-12T06:25:47Z

I think this might have caused this issue: #47079

It seems like there are two causes:

The Navigation block's List View tab isn't declared on first render, it only becomes present in the tabs array on later renders.
The TabPanel no longer supports lazily adding initial tabs after this PR, it now always falls back to the first enabled tab in the tabs array.

I did try looking to see if this could be fixed by always rendering the tab navigation block, but I couldn't find the source of the problem. It could be worth some further investigation though.

I've put together a fix for the TabPanel component - #47100.

joshuatf added 2 commits December 12, 2022 10:10

Add prop to allow disabling of a tab button

3c681f8

Add test for disabled property

f064cf1

joshuatf added the [Package] Components /packages/components label Dec 12, 2022

joshuatf self-assigned this Dec 12, 2022

joshuatf requested a review from ajitbohra as a code owner December 12, 2022 18:23

mirka requested review from brookewp, chad1008, ciampo and mirka December 12, 2022 20:36

joshuatf mentioned this pull request Dec 13, 2022

Disable irrelevant product tabs when variations exist woocommerce/woocommerce#35939

Merged

8 tasks

alexstine added [Type] Enhancement A suggestion for improvement. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Accessibility Feedback Need input from accessibility labels Dec 13, 2022

alexstine self-requested a review December 13, 2022 04:25

alexstine reviewed Dec 13, 2022

View reviewed changes

mirka reviewed Dec 13, 2022

View reviewed changes

joshuatf added 8 commits December 14, 2022 11:41

Add types to tabs and pick into button

3b0ea90

Simplify remaining props to TabButton

c92b370

Add disabled tab story

7177831

Add logic around setting active tab when tab is disabled

5296835

Add experimental focusable prop to button

662bfd7

Add tests around tab selection when disabled

46fc6b3

Add expectation around disabled selection via keyboard navigation

12953d0

Add changelog entry

6609175

mirka approved these changes Dec 21, 2022

View reviewed changes

mirka changed the title ~~Add prop to allow disabling of a tab button~~ TabPanel: Add prop to allow disabling of a tab button Dec 21, 2022

alexstine approved these changes Dec 22, 2022

View reviewed changes

Update TabPanel readme

b991862

ciampo reviewed Jan 2, 2023

View reviewed changes

Update handle selection logic to use else if

c64730c

ciampo approved these changes Jan 2, 2023

View reviewed changes

ciampo merged commit f6b2529 into trunk Jan 2, 2023

ciampo deleted the add/disabled-tab-prop branch January 2, 2023 21:03

github-actions bot added this to the Gutenberg 15.0 milestone Jan 2, 2023

madhusudhand mentioned this pull request Jan 3, 2023

TabPanel: add tabName prop (controlled component) #46704

Closed

talldan mentioned this pull request Jan 12, 2023

Fix tab panel initial tab selection #47100

Merged

Conversation

joshuatf commented Dec 12, 2022

What?

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

Uh oh!

alexstine left a comment

Choose a reason for hiding this comment

Uh oh!

mirka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joshuatf commented Dec 14, 2022

Uh oh!

joshuatf commented Dec 19, 2022

Uh oh!

mirka left a comment

Choose a reason for hiding this comment

Uh oh!

alexstine left a comment

Choose a reason for hiding this comment

Uh oh!

mirka commented Dec 23, 2022

Uh oh!

joshuatf commented Dec 29, 2022

Uh oh!

ciampo Jan 2, 2023

Choose a reason for hiding this comment

Uh oh!

joshuatf Jan 2, 2023

Choose a reason for hiding this comment

Uh oh!

ciampo left a comment

Choose a reason for hiding this comment

Uh oh!

scruffian commented Jan 11, 2023

Uh oh!

talldan commented Jan 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants