[Tabs] Add new experimental Tabs API #20806
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@material-ui/core: parsed: 0.00% 😍, gzip: +0.15% Details of bundle changes.Comparing: 939205f...a0af3d2 Details of page changes
|
This comment has been minimized.
This comment has been minimized.
|
@eps1lon Great work! One point of interrogation. How should we link a |
Right now they rely on the same position.
Please be more specific.
I'm not aware of this use case. Do you have a real-world example where you would conditionally hide a tab but not the corresponding tabpanel?
What problem and why do we need a second API for it? |
Sure, my point of interrogation is about, will the developers be able to correctly manage the positions of the children? In the below example, I would expect to work well, however considering #17452, and a more complex rendering logic, I wonder about the potential bug surface for off by 1 error it opens (developer not correctly syncing the shown Tab and TabPanel). <TabContext value={value}>
<AppBar position="static">
<TabList onChange={handleChange} aria-label="simple tabs example">
<Tab label="Item One" />
{foo === 'bar' ? <Tab label="Item Two" /> : null}
<Tab label="Item Three" />
</TabList>
</AppBar>
<TabPanels>
<TabPanel>Item One</TabPanel>
{foo === 'bar' ? <TabPanel>Item Two</TabPanel> : null}
<TabPanel>Item Three</TabPanel>
</TabPanels>
</TabContext>
It could enable the developers to break free from the component structure we impose and the dependencies these components have (JSS). |
For TabPanel we don't necessarily need it. I would expect the same customization options with any component though. But I wouldn't mind getting rid of
This is too constructed. The same problem existed before and without a concrete use case I'd consider this something we don't have to worry about. Developers that break out of the default use case could just as well wrap the component. There's definitely an opportunity to get rid of cloneElement which would require explicit |
Yeah, maybe It's a too far fetched case. It will be interesting to see the feedback we get once released and used.
It sounds like there is a hard tradeoff to make: https://gist.github.com/ryanflorence/10e9387f633f9d2e6f444a9bddaabf6e#descendants which resonates a bit with the different APIs tradeoffs: #20806 (comment). |
|
The thing is that we don't need to jump through all these hoops here. None of our demos require the composition. All of these "conditional rendering" or "lazy rendering" examples are (so far) constructed. This doesn't mean that there aren't use cases for grouping tabs with divs or separating elements. |
NMinhNguyen
reviewed
Apr 28, 2020
NMinhNguyen
reviewed
Apr 30, 2020
eps1lon
force-pushed
the
feat/A11yTab
branch
4 times, most recently
from
b799b7d to
1858844
Compare
|
Updated with new revision that simplifies the implementation by requiring a |
oliviertassinari
approved these changes
May 6, 2020
|
Let's get this in, work on |
oliviertassinari
added
the
component: tabs
oliviertassinari
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently you have to wire up two different sets of IDREFs to make the tabs accessible and make sure that values and indices are correctly set up.
This PR adds a couple of new components that implement the same pattern we used in our demos. With these components you don't need to wire up all the aria related props with the caveat of not server rendering those. However, we do require a
valueprop as a string now. Otherwise we would need to count the children.-- https://deploy-preview-20806--material-ui.netlify.app/components/tabs/#experimental-tabs-api
The new API only allows
stringasvalue. The API in core allows not setting any tab but there is no use case for this as a demo. It's also not allowed by ARIA and, frankly, I haven't seen a tabs UI where no panel is selected.Aside: The experimental useOpaqueIdentifier in react would not help here since we have to communicate these across the tree and create dynamically. Edit: See reactjs/rfcs#32 (comment)