vitamin-web
vitamin-web copied to clipboard
refactor(@vtmn/svelte, @vtmn/vue, @vtmn/react): rework tabs a11y
Changes description
Following rework on demo on #1233 (refactor(@vtmn/css): tabs demo showcase + rework a11y), some updates and fixes should be made on Svelte, React and Vue tabs integration.
- Components should observe a similar behavior as the demo, in particular regarding script for accessibility conformity.
- Add missing a11y attributes: aria, tabindex, ...
Context
- The
tabs
component wasn't accessible, now you can move across the different tabs with the left and right arrow keys. The tabs focus on the next element after the nav.
Checklist
- [x] Make sure you are requesting to pull a topic/feature/bugfix branch. Please, don't request directly from your main!
- [x] Check commits & PR names matches our requested structure. It must follow the https://www.conventionalcommits.org pattern.
- [x] Check your code additions will fail neither code linting checks.
- [x] I have reviewed the submitted code.
- [x] I have tested on related showcases.
- [x] If it includes design changes, please ask for a review with a core team designer.
Does this introduce a breaking change?
Yes :
- replace
a
withbutton
, remove thehref
attribute.
Hi @GaspardMathon @lauthieb @7studio @romuleald @Tlahey ,
I keep you updated on this PR. Fell free to answer, review or even ignore this message ;)
I pushed a new commit to challenge the current conception of our tabs. I started with the React integration.
As a reminder, here is what were missing :
- each tab item should be linked to a corresponding tab panel ;
- users should be able to tab from a tab item to the corresponding tabpanel, and use arrow keys to navigate through tab items ;
- missing a11y attributes should be fixed.
Unfortunately, all the fixes has resulted in a totally and completely breaking change :/ :
- user has now to provide tab panels ;
- accordingly, the tabs structure has changed since the items and the panels have to share some states variable now ;
- to keep the same strategy, major logic is hidden inside component, but user still have to provide some configuration. In addition to the already existing
VtmnTabsItem
, user has now to use 3 new components inside Tabs :VtmnTabsItems
,VtmnTabsPanels
,VtmnTabsPanel
(see below)
Regarding the integration of the component for React, here is some elements:
- The main idea was to keep the initial strategy, which was like the compound component pattern: leave to the user the rendering of the tabs navigation and tab panels, the easiest way possible, and hide some internal logic inside the component. I'm not sure if it is the best pattern for this case (we could just have a tab component with a content prop, waiting for an array or an object...), but once again the main idea was to keep current implementation logic.
- Hence, I kept this idea by defining 2 « areas », meaning a component
VtmnTabsItems
and a componentVtmnTabsPanels
, instead of a singlechildren
for items. Both of these « areas » have their own children than the user can fill out, usually with theVtmnTabsItem
and a newVtmnTabsPanel
See:
<VtmnTabs>
<VtmnTabsItems>
<VtmnTabsItem>Tab Item 1</VtmnTabsItem>
<VtmnTabsItem>Tab Item 2</VtmnTabsItem>
<VtmnTabsItem>Tab Item 3</VtmnTabsItem>
</VtmnTabsItems>
<VtmnTabsPanels>
<VtmnTabsPanel>Tab Panel 1</VtmnTabsPanel>
<VtmnTabsPanel>Tab Panel 2</VtmnTabsPanel>
<VtmnTabsPanel>Tab Panel 3</VtmnTabsPanel>
</VtmnTabsPanels>
</VtmnTabs>
- This way all the logic is kept inside component, out of reach for the user. Tabs component shares global state (currentTabIndex, …) to both Items area (
VtmnTabsItems
) and panels area (VtmnTabsPanels
). Each of these zone manages their own local state: for instanceVtmnTabsItems
manages the arrow navigation and gives its items some props such as 'aria-controls' according to their index ;VtmnTabsPanels
gives its panels some props such as 'aria-labelledby' according to their index. Finally VtmnTabsItem and VtmnTabsPanel are mainly UI - Regarding this implementation,
Now, 2 question remain:
- What do you guys think about keeping the current compound component pattern? I know that it is not the best implementation for React right now (for instance it could be a good idea to use the context API for sharing some state elements here). How do you feel for the Svelte and Vue implementation?
- Because of all the breaking changes, I think that we should wait for the complete rework of the whole component on Vitamin V1. This would allow us to provide all user with better accessibility and the best experience. What do you think?
Thanks :)
Thanks for the news @thibault-mahe. I agree with all the things you've said. I think that we should let this PR in draft and we will rework the component for the next major version.
We had a similar strategy with the vtmn-text-input
: a lot of improvements could be made but were breaking change so we raised an issue not to forget to update this component for the next version.
Sorry to disturb this PR with an old subject but you work too fast.
After our first conversation about the markup of this component (<nav>
was deleted) I had some doubts about the <ul role="tablist"><li><button role="tab">
too.
According to this thread on Twiiter: https://twitter.com/7studio/status/1565425932923437056 with role="tablist"
, the <ul>
element isn't a list anymore and the <li>
blocks a correct restitution with its role listitem
.
The right markup would be: <ul role="tablist"><li role="none"><button role="tabt">
or simplier <div role="tablist"><button role="tab">
I hope it helps to improve your component.
Hi @7studio, thanks for your suggestion. Indeed I didn't know that the role tablist
removes the list quality of the ul
. The thread you shared is really informative on the subject. I agree with the simpler <div role="tablist"><button role="tab">
, we'll make a fix accordingly.
Thanks!
Thanks @7studio, I've made an update in consequence ;)