(Fix): Whitespace in Tab Labels #5284
Conversation
|
There was a problem hiding this comment.
This looks good. Could we add unit tests for the updates? You probably want to reference how spyOn works in jest https://jestjs.io/docs/jest-object#jestspyonobject-methodname. Then you are able to test the call to useStableId and mock its return value.
|
@zchenwei I added tests to confirm that the IDs are unique and that they follow the naming convention to end in '-tab'. |
|
Can you please add an HTML output example in the description to show the full id that's generated? |
| id={`${value}-panel`} | ||
| aria-labelledby={`${value}-tab`} | ||
| id={`${idValue}-panel`} | ||
| aria-labelledby={`${idValue}-tab`} |
There was a problem hiding this comment.
If we are using useStableId in 2 different components, wouldn't that generate 2 different IDs? We need aria-labelledby in this component to be the id of the TabItem and vice-versa with aria-controls and this id
There was a problem hiding this comment.
I switched it back to using the given value, with spaces replaced with - to ensure the all of the labels work cohesively. This relies on the user giving each tab a unique value.
There was a problem hiding this comment.
Can we think of another solution to providing a unique id? Maybe passing a React.useId or useStableId down through context so it is shared? @zchenwei any thoughts on that?
I think it creates sort of a "hidden" requirement that each value has to be unique because it is also happens to be used to generate the ID. It also creates a scenario where the developer has to track down the cause of duplicate IDs or potentially inaccessible markup. This should be something we can safely back into the component. An example is if you inspect that markup and usage of the Radix Tabs component https://www.radix-ui.com/primitives/docs/components/tabs
There was a problem hiding this comment.
Looking at the Radix implementation, they still rely on the user-inputed value to create their full IDs. They feed the value into functions to create the trigger ID and the content ID, both based off of one baseID for the entire tab group. In their implementation, so if the user chose to input the same value for multiple tabs in the same group they'd run into the same issue. What their solution does address is having multiple tab groups on the same page, because the baseID will be different, and thus two tabs with value "Tab 1" will result in trigger ids: {baseID}-trigger-Tab 1, where the baseID will be different.
Essentially, it looks they are guaranteeing that so long as you declare unique values for each tab in a tab list, you won't run into any issues, but I believe that there is still that dependance on the user entering unique values.
There was a problem hiding this comment.
Right, the issue is not so much with tabs within the same tab group, but for multiple Tab components in the DOM at the same time that use the same tab names. Within the same tab group, I think it's reasonable that users should specify unique tab names. But for multiple sets of the same tab names on a page, that should be something we can build into this component by generating a unique ID (like the baseId that Radix generates) that is used to build out the ID string.
An example of this is our Docs site that sometimes uses multiple tab components on a page, like the docs for adding Facebook login, Google login, etc.
…ing with useStableId()
Description of changes
Generates ID without whitespace for each tab component.
Example: A tab declared with
value="Tab 1"produces the following HTML output<button role="tab" id="Tab-1-tab" aria-selected="true" aria-controls="Tab-1-panel" class="amplify-tabs__item amplify-tabs__item--active">Tab 1</button>Issue #, if available
Fixes #5220
Description of how you validated changes
Inspected elements in Tabs Docs, IDs now contain no whitespace and are unique.
Checklist
yarn testpasses and tests are updated/addedsideEffectsfield updatedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.