feat: Tabs collapse behaviour #7202

snowystinger · 2024-10-16T08:06:03Z

Closes

✅ Pull Request Checklist:

Included link to corresponding React Spectrum GitHub Issue.
Added/updated unit tests and storybook for this change (for new code or code which already has tests).
Filled out test instructions.
Updated documentation (if it already exists for this component).
Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

packages/@react-spectrum/s2/src/Tabs.tsx

packages/react-aria-components/src/Breadcrumbs.tsx

packages/react-aria-components/src/Tabs.tsx

rspbot · 2024-10-17T08:10:15Z

Build successful! 🎉

rspbot · 2024-10-21T05:58:32Z

Build successful! 🎉

rspbot · 2024-10-22T04:28:27Z

Build successful! 🎉

rspbot · 2024-10-22T04:42:10Z

Build successful! 🎉

packages/@react-spectrum/s2/src/Tabs.tsx

LFDanLu

Partial review, will finish Monday

packages/@react-spectrum/s2/stories/Tabs.stories.tsx

packages/@react-spectrum/s2/src/Tabs.tsx

LFDanLu · 2024-10-25T00:28:19Z

packages/@react-spectrum/s2/src/Tabs.tsx

      let tab: HTMLElement | null = tablistRef.current.querySelector('[role=tab][data-selected=true]');

      if (tab != null) {
        setSelectedTab(tab);
      }
+    } else if (tablistRef?.current) {
+      let picker: HTMLElement | null = tablistRef.current.querySelector('button');


I forget if aria disallows it, but is the picker guaranteed to be the only button element in the tablist? I don't see children presentation for the tablist in the spec

yeah, the role tablist doesn't allow for anything other than tabs, I may need to switch the role when we collapse now that you bring it up...

I just looked at S1 Tabs, looks like we don't change the role of the picker but I see in https://w3c.github.io/aria/#tablist does explicitly state "tab" as the only allowed child role like you mentioned... cc @majornista

@majornista thoughts?

packages/@react-spectrum/s2/src/Tabs.tsx

packages/@react-spectrum/s2/src/TabsPicker.tsx

rspbot · 2024-11-04T16:37:02Z

Build successful! 🎉

rspbot · 2024-11-04T19:13:31Z

Build successful! 🎉

# Conflicts: # packages/@react-spectrum/s2/stories/Tabs.stories.tsx

rspbot · 2024-11-20T04:16:47Z

Build successful! 🎉

# Conflicts: # packages/@react-spectrum/s2/package.json # yarn.lock

rspbot · 2024-11-21T04:16:02Z

Build successful! 🎉

reidbarber

LGTM

# Conflicts: # packages/@react-spectrum/s2/src/Tabs.tsx

rspbot · 2024-11-27T19:58:01Z

Build successful! 🎉

LFDanLu

Approving for testing, happy for the rest to be followup

LFDanLu · 2024-11-27T21:41:33Z

packages/@react-spectrum/s2/src/Tabs.tsx

+            // need to determine the best way to handle icon only -> icon and text
+            // good enough to aria-label the picker item?


An interesting situation can happen with the icon only tabs: if you collapse https://reactspectrum.blob.core.windows.net/reactspectrum/777f4b761ccd6a3e45cbdd9d8ee4d623ee6e3516/storybook-s2/index.html?path=/story/tabs--icons&providerSwitcher-locale=he-IL, you will never be able to see the full contents of two of the options because the point of collapse for the icon only tabs is too small. This maybe partially mitigated by the work you are doing to support tooltips on collection items, but maybe this means we should only render the icon of the tab even when it is collapsed but display the full text in the menu dropdown like it already does. Alternatively, we could instead just have the text truncate so it doesn't get cut off

Happy for this to be followup and for others to weigh in.

I don't quite follow, are you talking about how the picker shows the label and runs off the overflow area? So the picker should only have the icon?
here's the two options I was trying to decide between

The reason I went the direction I did was so it would more easily label, but I could change it to have the display none text aria-label the icon possibly.

Yep, you basically got the gist of it, essentially the user doesn't have a great way of seeing the selected tab's full text other than opening the dropdown. IMO option 2 that you show above isn't too bad for sighted users since the non collapsed tabs already didn't show the tab's text and aria-labeling would work for screen reader users.

LFDanLu · 2024-11-27T21:43:20Z

packages/@react-spectrum/s2/src/Tabs.tsx

+  );
+}
+
+function isAllTabsDisabled<T>(collection: Collection<Node<T>> | undefined, disabledKeys: Set<Key>) {


Interesting that the selectionManager/collection can't help in a case like this where the individual items might be disabled. Maybe worth making RAC tabs return this info in the render props or something

mmm possibly
yeah, the selectionManager is unaware of it, the collection could do it, but I'm not sure if it should because 'all' is a little subjective to the collection being async or not

it'd be more like, 'all currently included items disabled' or something

yep, perhaps something we can discuss/handle as a follow up if others using RAC tabs find themselves needing similar logic

packages/@react-spectrum/s2/src/TabsPicker.tsx

LFDanLu · 2024-11-27T21:57:15Z

packages/@react-spectrum/s2/src/TabsPicker.tsx

Didn't notice this before, but it seems like focus is lost if you keyboard select an option from the tab picker menu. Also getting warning about missing a heading in the console

huh... i swear i tested this, however, it looks broken in every commit since I added the picker, so not sure what happened. looking into it

focusscope thinks it's restoring to the correct element, the trigger remains in the dom...

ah, react-aria-focus-scope-restore is preventing default off in useSelectableCollection for the tablist, but it doesn't know how to focus the picker

ok, i've addressed it in Tabs, not sure if it should be addressed further down

oh interesting, doesn't feel like useSelectableCollection should be preventing default in this case. Might be the same issue mentioned here: #6451 (comment), lets discuss in a follow up

yihuiliao

small comments which can be follow-up

packages/@react-spectrum/s2/src/Tabs.tsx

packages/@react-spectrum/s2/src/TabsPicker.tsx

rspbot · 2024-11-29T00:10:11Z

Build successful! 🎉

rspbot · 2024-11-29T00:10:12Z

## API Changes

@react-spectrum/s2

/@react-spectrum/s2:Tabs

 Tabs {
   UNSAFE_className?: string
   UNSAFE_style?: CSSProperties
   aria-describedby?: string
   aria-details?: string
   aria-label?: string
   aria-labelledby?: string
   children?: ReactNode
   defaultSelectedKey?: Key
   density?: 'compact' | 'regular' = 'regular'
   disabledKeys?: Iterable<Key>
+  iconOnly?: boolean
   id?: string
   isDisabled?: boolean
   keyboardActivation?: 'automatic' | 'manual' = 'automatic'
   onSelectionChange?: (Key) => void
   selectedKey?: Key | null
   slot?: string | null
   styles?: StylesPropWithHeight
 }

/@react-spectrum/s2:TabList

 TabList <T extends {}> {
   UNSAFE_className?: string
   UNSAFE_style?: CSSProperties
   aria-describedby?: string
   aria-details?: string
   aria-label?: string
   aria-labelledby?: string
-  children?: ReactNode
+  children?: ReactNode | (T) => ReactNode
   dependencies?: Array<any>
   items?: Iterable<T>
   styles?: StylesProp
 }

/@react-spectrum/s2:Tab

 Tab {
   UNSAFE_className?: string
   UNSAFE_style?: CSSProperties
   aria-describedby?: string
   aria-details?: string
   aria-label?: string
   aria-labelledby?: string
-  children?: ReactNode
+  children: ReactNode
   download?: boolean | string
   href?: Href
   hrefLang?: string
   id?: Key
   onHoverChange?: (boolean) => void
   onHoverEnd?: (HoverEvent) => void
   onHoverStart?: (HoverEvent) => void
   ping?: string
   referrerPolicy?: HTMLAttributeReferrerPolicy
   rel?: string
   routerOptions?: RouterOptions
   styles?: StylesProp
   target?: HTMLAttributeAnchorTarget
 }

/@react-spectrum/s2:TabsProps

 TabsProps {
   UNSAFE_className?: string
   UNSAFE_style?: CSSProperties
   aria-describedby?: string
   aria-details?: string
   aria-label?: string
   aria-labelledby?: string
   children?: ReactNode
   defaultSelectedKey?: Key
   density?: 'compact' | 'regular' = 'regular'
   disabledKeys?: Iterable<Key>
+  iconOnly?: boolean
   id?: string
   isDisabled?: boolean
   keyboardActivation?: 'automatic' | 'manual' = 'automatic'
   onSelectionChange?: (Key) => void
   selectedKey?: Key | null
   slot?: string | null
   styles?: StylesPropWithHeight
 }

/@react-spectrum/s2:TabProps

 TabProps {
   UNSAFE_className?: string
   UNSAFE_style?: CSSProperties
   aria-describedby?: string
   aria-details?: string
   aria-label?: string
   aria-labelledby?: string
-  children?: ReactNode
+  children: ReactNode
   download?: boolean | string
   href?: Href
   hrefLang?: string
   id?: Key
   onHoverChange?: (boolean) => void
   onHoverEnd?: (HoverEvent) => void
   onHoverStart?: (HoverEvent) => void
   ping?: string
   referrerPolicy?: HTMLAttributeReferrerPolicy
   rel?: string
   routerOptions?: RouterOptions
   styles?: StylesProp
   target?: HTMLAttributeAnchorTarget
 }

/@react-spectrum/s2:TabListProps

 TabListProps <T> {
   UNSAFE_className?: string
   UNSAFE_style?: CSSProperties
   aria-describedby?: string
   aria-details?: string
   aria-label?: string
   aria-labelledby?: string
-  children?: ReactNode
+  children?: ReactNode | (T) => ReactNode
   dependencies?: Array<any>
   items?: Iterable<T>
   styles?: StylesProp
 }

This reverts commit cd4da2b.

* Revert "chore: Update JSDocs and apis for release (#7595)" This reverts commit 101d077. * Revert "fix: Tabs infinite loop (#7487)" This reverts commit 8228e4e. * Revert "fix: Tabs from testing (#7463)" This reverts commit 494e01c. * Revert "feat: Tabs collapse behaviour (#7202)" This reverts commit cd4da2b. * add stuff back that we still want * update lock

feat: Tabs collapse behaviour

c81ba45

snowystinger commented Oct 16, 2024

View reviewed changes

packages/@react-spectrum/s2/src/Tabs.tsx Outdated Show resolved Hide resolved

snowystinger commented Oct 16, 2024

View reviewed changes

packages/react-aria-components/src/Breadcrumbs.tsx Show resolved Hide resolved

snowystinger commented Oct 16, 2024

View reviewed changes

packages/react-aria-components/src/Tabs.tsx Show resolved Hide resolved

snowystinger added 2 commits October 17, 2024 18:48

Get picker menu working

f8cf117

fix all the lint

d46d15a

Support orientation changes, fix sizing

020392b

snowystinger changed the title ~~[WIP] feat: Tabs collapse behaviour~~ feat: Tabs collapse behaviour Oct 21, 2024

snowystinger marked this pull request as ready for review October 21, 2024 04:53

snowystinger added 2 commits October 21, 2024 16:19

fix lint

2728e02

reduce renders, tix ts

4dd9b00

snowystinger added 2 commits October 22, 2024 15:08

support iconOnly with collapsing

99c5a70

fix frickin lint

9214709

Merge branch 'main' into tabs-collapse

e76be75

snowystinger added the in sprint label Oct 22, 2024

reidbarber reviewed Oct 23, 2024

View reviewed changes

packages/@react-spectrum/s2/src/Tabs.tsx Show resolved Hide resolved

LFDanLu reviewed Oct 25, 2024

View reviewed changes

yihuiliao reviewed Oct 31, 2024

View reviewed changes

packages/@react-spectrum/s2/src/Tabs.tsx Show resolved Hide resolved

yihuiliao reviewed Oct 31, 2024

View reviewed changes

packages/@react-spectrum/s2/src/TabsPicker.tsx Show resolved Hide resolved

yihuiliao reviewed Oct 31, 2024

View reviewed changes

packages/@react-spectrum/s2/src/TabsPicker.tsx Outdated Show resolved Hide resolved

yihuiliao reviewed Oct 31, 2024

View reviewed changes

packages/@react-spectrum/s2/src/TabsPicker.tsx Outdated Show resolved Hide resolved

fix TS

508d51a

snowystinger and others added 3 commits November 4, 2024 11:17

Merge branch 'main' into tabs-collapse

e6f3d40

remove unused context

09b5cb2

pass along disabled keys

b22b6e9

Merge branch 'main' into tabs-collapse

9a8edd6

# Conflicts: # packages/@react-spectrum/s2/stories/Tabs.stories.tsx

snowystinger added 2 commits November 21, 2024 15:00

Merge branch 'main' into tabs-collapse

9d5fce1

# Conflicts: # packages/@react-spectrum/s2/package.json # yarn.lock

fix lint

6466f1c

reidbarber previously approved these changes Nov 22, 2024

View reviewed changes

Merge branch 'main' into tabs-collapse

777f4b7

# Conflicts: # packages/@react-spectrum/s2/src/Tabs.tsx

snowystinger dismissed reidbarber’s stale review via 777f4b7 November 27, 2024 19:48

reidbarber previously approved these changes Nov 27, 2024

View reviewed changes

LFDanLu previously approved these changes Nov 27, 2024

View reviewed changes

yihuiliao previously approved these changes Nov 27, 2024

View reviewed changes

packages/@react-spectrum/s2/src/Tabs.tsx Outdated Show resolved Hide resolved

packages/@react-spectrum/s2/src/TabsPicker.tsx Show resolved Hide resolved

packages/@react-spectrum/s2/src/TabsPicker.tsx Outdated Show resolved Hide resolved

review comments

5caf3ea

snowystinger dismissed stale reviews from yihuiliao, LFDanLu, and reidbarber via 5caf3ea November 27, 2024 23:41

snowystinger and others added 3 commits November 29, 2024 08:44

Merge branch 'main' into tabs-collapse

e261fcd

more review comments

4ac5cf5

Fix restore focus

34d7617

yihuiliao approved these changes Dec 2, 2024

View reviewed changes

LFDanLu approved these changes Dec 2, 2024

View reviewed changes

LFDanLu added this pull request to the merge queue Dec 2, 2024

Merged via the queue into main with commit cd4da2b Dec 2, 2024
30 checks passed

LFDanLu deleted the tabs-collapse branch December 2, 2024 18:54

LFDanLu added a commit that referenced this pull request Jan 11, 2025

Revert "feat: Tabs collapse behaviour (#7202)"

c5d4bd6

This reverts commit cd4da2b.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Tabs collapse behaviour #7202

feat: Tabs collapse behaviour #7202

snowystinger commented Oct 16, 2024

rspbot commented Oct 17, 2024

rspbot commented Oct 21, 2024

rspbot commented Oct 22, 2024

rspbot commented Oct 22, 2024

LFDanLu left a comment

LFDanLu Oct 25, 2024

snowystinger Oct 28, 2024

LFDanLu Oct 28, 2024

snowystinger Nov 21, 2024

rspbot commented Nov 4, 2024

rspbot commented Nov 4, 2024

rspbot commented Nov 20, 2024

rspbot commented Nov 21, 2024

reidbarber left a comment

rspbot commented Nov 27, 2024

LFDanLu left a comment

LFDanLu Nov 27, 2024

snowystinger Nov 28, 2024

LFDanLu Dec 2, 2024

LFDanLu Nov 27, 2024

snowystinger Nov 28, 2024

LFDanLu Dec 2, 2024

LFDanLu Nov 27, 2024

snowystinger Nov 28, 2024 •

edited

Loading

snowystinger Nov 28, 2024

LFDanLu Dec 2, 2024

yihuiliao left a comment

rspbot commented Nov 29, 2024

rspbot commented Nov 29, 2024

		// need to determine the best way to handle icon only -> icon and text
		// good enough to aria-label the picker item?

feat: Tabs collapse behaviour #7202

feat: Tabs collapse behaviour #7202

Conversation

snowystinger commented Oct 16, 2024

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

rspbot commented Oct 17, 2024

rspbot commented Oct 21, 2024

rspbot commented Oct 22, 2024

rspbot commented Oct 22, 2024

LFDanLu 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

rspbot commented Nov 4, 2024

rspbot commented Nov 4, 2024

rspbot commented Nov 20, 2024

rspbot commented Nov 21, 2024

reidbarber left a comment

Choose a reason for hiding this comment

rspbot commented Nov 27, 2024

LFDanLu 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

snowystinger Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuiliao left a comment

Choose a reason for hiding this comment

rspbot commented Nov 29, 2024

rspbot commented Nov 29, 2024

@react-spectrum/s2

/@react-spectrum/s2:Tabs

/@react-spectrum/s2:TabList

/@react-spectrum/s2:Tab

/@react-spectrum/s2:TabsProps

/@react-spectrum/s2:TabProps

/@react-spectrum/s2:TabListProps

snowystinger Nov 28, 2024 •

edited

Loading