Add `Tab` and `TabList` components #813

lyzadanger · 2023-01-27T21:09:01Z

This PR adds new Tab and TabList components, and pattern-library documentation for same.

Why now: I believe we may wish to use a tabbed interface in upcoming new UI in the client and before building out new tabs, I wanted to:

Extract the design pattern: Styling is based on the sidebar tabs (SelectionTabs) in the client, and extraction helps to better encapsulate the design pattern vs. the SelectionsTab-specific logic.
Ensure that we are ticking accessibility boxes. The new TabList adds arrow-key navigation per https://www.w3.org/WAI/ARIA/apg/patterns/tabpanel/
Fix a longstanding layout complexity to do with tabs jiggling when selected because of bold text. We've put various workarounds in place in the client over the years to try to account for it, but the new Tab component takes aim at alleviating it systematically (see documentation on textContent for that component).

As always with these new-component PRs, the lion's share of the diff is the pattern-library documentation page component. The components themselves were refreshingly simple to put together: I was able to scaffold out and concentrate on the specific problems I wanted to solve (fixing the jiggle, applying keyboard navigation) and documentation. The toolbox is getting more robust and it's starting to pay dividends!

Testing

Check out this branch, run make dev and visit the Tabs page in the pattern library

lyzadanger · 2023-01-27T21:09:43Z

src/components/navigation/Tab.tsx

+          // Make the ::before occupy space within the button, but make it
+          // invisible. This ensures that the tab button is wide enough to show
+          // bolded text even if the visible text is not currently bold. See
+          // https://css-tricks.com/bold-on-hover-without-the-layout-shift/


The buttons at the top of this interface itself use this trick so that they don't move around when active:

Clever! 😃

lyzadanger · 2023-01-27T21:10:57Z

src/components/navigation/test/Tab-test.js

+const contentFn = (Component, props = {}) => {
+  return mount(
+    <div role="tablist">
+      <Component {...props}>This is child content</Component>
+    </div>
+  );
+};


Here and in the tests for TabList: our automated common and accessibility tests are working well! Accessibility tests will fail if Tab isn't inside of a tablist, and TabList tests will fail if the TabList doesn't have any tab children. Sweet.

codecov · 2023-01-30T13:40:28Z

Codecov Report

Merging #813 (69760ef) into main (4862843) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 69760ef differs from pull request most recent head 932564a. Consider uploading reports for the commit 932564a to get more accurate results

@@            Coverage Diff            @@
##              main      #813   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           52        54    +2     
  Lines          633       649   +16     
  Branches       225       233    +8     
=========================================
+ Hits           633       649   +16

Impacted Files	Coverage Δ
src/components/navigation/Tab.tsx	`100.00% <100.00%> (ø)`
src/components/navigation/TabList.tsx	`100.00% <100.00%> (ø)`
src/components/input/Checkbox.js	`100.00% <0.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

lyzadanger · 2023-01-30T18:00:22Z

I've gone ahead and pinged both @robertknight and @acelaya for review on this one. Rob more specifically for the keyboard navigation and behavior aspects.

acelaya

A couple of minor comments, but looks good 👍🏼

acelaya · 2023-01-31T08:17:03Z

src/components/navigation/Tab.tsx

+   */
+  textContent?: string;
+  selected?: boolean;
+  variant?: 'basic';


I understand the intention is to have more variants eventually? Maybe I would suggest not adding this property until we have other variants?

I was on the fence about this. Tell you what: with this UI prototyping, I'm exercising these components quite a lot (it led me to add icon support to Tab and vertical support to TabList). I'm starting to lean toward the component itself doing no styling other than layout. I promise to address this in a follow-up PR.

acelaya · 2023-01-31T08:19:52Z

src/components/navigation/Tab.tsx

+          // Make the ::before occupy space within the button, but make it
+          // invisible. This ensures that the tab button is wide enough to show
+          // bolded text even if the visible text is not currently bold. See
+          // https://css-tricks.com/bold-on-hover-without-the-layout-shift/


Clever! 😃

robertknight

The implementation fundamentals look good. From a design perspective I think in future we may want to reconsider the lack of any borders between tabs or between tabs and tab panels, as this makes the tab-iness of the control less obvious, at least in certain contexts.

robertknight · 2023-01-31T11:12:35Z

src/components/navigation/TabList.tsx

+  useArrowKeyNavigation(tabListRef, {
+    selector: 'button',
+    horizontal: true,
+    vertical: vertical,


Suggested change

vertical: vertical,

vertical,

robertknight · 2023-01-31T11:17:22Z

src/pattern-library/components/patterns/navigation/TabPage.js

+                <code>aria-controls</code>
+              </a>{' '}
+              attribute to each <code>Tab</code>. This is not always feasible in
+              our applications.


My initial reaction to "This is not always feasible in our applications" was "why?". Are we doing something unusual or is there an implementation problem? What are the problems caused by not using this?

Great question! The WAI-ARIA authoring guidelines for a full tabby-widget-y interface assumes a structure like so:

<tablist> <tab controls="panel1">One</tab> <tab controls="panel2" aria-selected>Two</tab> <tab controls="panel3">Three</tab> </tablist> <tabpanel id="panel1" style="visibility:hidden">Content for panel One</tabpanel> <tabpanel id="panel2">Content for panel Two</tabpanel> <tabpanel id="panel3" style="visibility:hidden">Content for panel Three</tabpanel>

To wit:

A tabpanel for each tab

Non-active tabpanels are visually hidden

However, in the sidebar, we have:

<tablist> <tab>Annotations</tab> <tab aria-selected>Page Notes</tab> <tab>Orphans</tab> </tablist> <div>Page-note annotation threads</div>

To wit: All three tabs control the same "panel"; we don't have a discrete element "per panel". I don't think it would appropriate to assign the same aria-controls value to each tab (i.e. the id of the div containing threads, which, for clarity, does not currently have an id attribute). But I could be wrong.

I think this sort of situation is why WAI-ARIA guidelines indicate that there should be an aria-controls on role="tab" elements, but does not indicate that there must.

To wit: All three tabs control the same "panel"; we don't have a discrete element "per panel".

Ah, OK. That should be an implementation detail that we are re-using the DOM elements though. From a user's perspective (screen reader or not) I think it would ideally "look" the same as if we were using separate panels. I wonder if there is a blessed way of doing this kind of virtualization. How does aria-controls actually get used?

robertknight · 2023-01-31T11:23:50Z

src/pattern-library/components/patterns/navigation/TabPage.js

+                  Annotations
+                  <span className="relative bottom-[3px] left-[2px] text-[10px]">
+                    {52}
+                  </span>


Do you have plans to create a pattern library component for counts associated with a tab? I suspect this may be a common-enough pattern and have enough considerations around design/a11y that require thought to make it worthwhile.

We could do that, sure. It would simplify the acrobatics I did in re textContent. I'll give it a think.

robertknight · 2023-01-31T11:26:32Z

src/components/navigation/Tab.tsx

+   * button will be sized to accommodate this string in bold text. This can
+   * prevent tab jiggle.
+   */
+  textContent?: string;


I suppose we could try to simplify the API by determining the textContent automatically, eg. in a useLayoutEffect that reads textContent after render, although that would have a limitation of not being able to account for expected text changes (eg. if counts were not available and become available later), and be a bit more expensive.

Yeah, I'm going to continue to hem and haw on this some. I don't plan to immediately cut a release of the package, so something intelligent might occur to me in the next few days.

lyzadanger · 2023-01-31T13:08:47Z

The implementation fundamentals look good. From a design perspective I think in future we may want to reconsider the lack of any borders between tabs or between tabs and tab panels, as this makes the tab-iness of the control less obvious, at least in certain contexts.

For sure. My goal in the "styling" part of this was simply to capture the current pattern as-is.

lyzadanger commented Jan 27, 2023

View reviewed changes

lyzadanger requested a review from robertknight January 27, 2023 21:13

lyzadanger force-pushed the tabs branch from e647f6f to c587ecd Compare January 30, 2023 13:38

lyzadanger force-pushed the tabs branch from c587ecd to 69760ef Compare January 30, 2023 17:58

lyzadanger requested a review from acelaya January 30, 2023 17:59

acelaya approved these changes Jan 31, 2023

View reviewed changes

robertknight approved these changes Jan 31, 2023

View reviewed changes

lyzadanger added 2 commits January 31, 2023 08:37

Add Tab and TabList components

f37af3a

Add pattern-library documentation for Tab and TabList

932564a

lyzadanger force-pushed the tabs branch from 69760ef to 932564a Compare January 31, 2023 13:39

lyzadanger merged commit c783276 into main Jan 31, 2023

lyzadanger deleted the tabs branch January 31, 2023 13:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Tab` and `TabList` components #813

Add `Tab` and `TabList` components #813

lyzadanger commented Jan 27, 2023 •

edited

Loading

lyzadanger Jan 27, 2023

acelaya Jan 31, 2023

lyzadanger Jan 27, 2023

codecov bot commented Jan 30, 2023 •

edited

Loading

lyzadanger commented Jan 30, 2023

acelaya left a comment

acelaya Jan 31, 2023

lyzadanger Jan 31, 2023

acelaya Jan 31, 2023

robertknight left a comment

robertknight Jan 31, 2023

robertknight Jan 31, 2023

lyzadanger Jan 31, 2023

robertknight Jan 31, 2023

robertknight Jan 31, 2023

lyzadanger Jan 31, 2023

robertknight Jan 31, 2023

lyzadanger Jan 31, 2023

lyzadanger commented Jan 31, 2023

Add Tab and TabList components #813

Add Tab and TabList components #813

Conversation

lyzadanger commented Jan 27, 2023 • edited Loading

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 30, 2023 • edited Loading

Codecov Report

lyzadanger commented Jan 30, 2023

acelaya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertknight left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyzadanger commented Jan 31, 2023

Add `Tab` and `TabList` components #813

Add `Tab` and `TabList` components #813

lyzadanger commented Jan 27, 2023 •

edited

Loading

codecov bot commented Jan 30, 2023 •

edited

Loading