-
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
TabPanel: Add prop to allow disabling of a tab button #46471
Conversation
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.
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
.
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.
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.
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
Please let me know if everything looks good now- thanks! |
Hi @mirka @alexstine - could you give this PR another look when you have time? Thanks! |
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.
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.
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.
@joshuatf Sorry for the delay, I have been a little under the weather lately.
Looks good.
Almost forgot, let's update the component readme before we merge. |
Thank you both for the reviews, @alexstine and @mirka! Updated the readme in b991862. |
} | ||
}, [ tabs, selectedTab?.name, initialTabName, handleTabSelection ] ); | ||
if ( selectedTab?.disabled && firstEnabledTab ) { |
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.
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?
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.
Sure! Done in c64730c.
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.
Thank you for working on this! 🎉
I think this might have caused this issue: #47079 |
It seems like there are two causes:
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 |
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
npm run test:unit tab-panel
and make sure all tests passTesting Instructions for Keyboard
Screenshots or screencast