fix(ui-react): Whitespace in Tab IDs #5378
Conversation
🦋 Changeset detectedLatest commit: 7a9bcdd The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| id={`${groupId}-tab-${idValue}`} | ||
| aria-selected={isActive} | ||
| aria-controls={`${value}-panel`} | ||
| aria-controls={`${groupId}-panel-${idValue}`} |
There was a problem hiding this comment.
If idValue is undefined then the values used on lines 38 and 40 will not be unique
There was a problem hiding this comment.
value for TabsPanel and TabsItem is required to be a string
There was a problem hiding this comment.
From the testing I've done this change does not seem to be breaking. At this stage of the Tabs composition value is required to be defined, so creating idValue based on it does not seem to introduce any errors. Because value itself is not changed, as long as defaultValue matches the intended target Tab's value the controllability and default functionality seems to work as intended. Tested by changing value and defaultValue for the examples to include whitespace.
Adds
groupIdto TabsContext such that unique IDs can be created for each tab group. IDs are of the form${groupId}-tab-${idValue}for tabs, and${groupId}-panel-${idValue}for panels.idValueis the givenvaluestripped of its whitespace using the added constantWHITESPACE_VALUE.idValueis created in TabsPanel and TabsItem, so assuming the developer enters the same values for these two elements the functionality works.defaultValueand controlled tab functionality still works because the actualvalueof the tabs is not altered in any way, but it is highly recommended to not assignvalueto a string containing whitespace.Outer html output for a tablist with given values: ['Tab 1', 'Tab 2', 'Tab 3']:
<div role="tablist" class="amplify-tabs__list" style="justify-content: flex-start;"><button role="tab" id="amplify-id-:rl:-tab-Tab-1" aria-selected="true" aria-controls="amplify-id-:rl:-panel-Tab-1" class="amplify-tabs__item amplify-tabs__item--active">Tab 1</button><button role="tab" id="amplify-id-:rl:-tab-Tab-2" aria-selected="false" aria-controls="amplify-id-:rl:-panel-Tab-2" tabindex="-1" class="amplify-tabs__item">Tab 2</button><button disabled="" role="tab" id="amplify-id-:rl:-tab-Tab-3" aria-selected="false" aria-controls="amplify-id-:rl:-panel-Tab-3" tabindex="-1" class="amplify-tabs__item">Disabled tab</button></div>Also updated the ComposedTabsExample.tsx file such that each Tab has unique content (previously each was "Tab 1 Content").
Issue #, if available
#5220
#5302 is an old version of this PR that had errors
Description of how you validated changes
Added tests to confirm IDs are the same for tabs in the same tab group with the same given value (should be avoided in practice), IDs are different for tabs in different groups with the same value (fine to do in practice). Also confirms that tabs in same group with different values yield unique IDs. Passes all yarn tests.
Also, edited the Amplify UI Docs examples to explore cases where
valueanddefaultValueare passed with whitespace to ensure composability, control, and disable still work as intended. User experience perfectly matches the production version of Amplify UI Docs.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.