Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Revert tabs collapse #7599

Merged
merged 6 commits into from
Jan 13, 2025
Merged

chore: Revert tabs collapse #7599

merged 6 commits into from
Jan 13, 2025

Conversation

LFDanLu
Copy link
Member

@LFDanLu LFDanLu commented Jan 11, 2025

The S2 tabs collapse PR had a picker within the tablist with isn't valid ARIA so reverting for now

✅ 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:

@rspbot
Copy link

rspbot commented Jan 11, 2025

@rspbot
Copy link

rspbot commented Jan 11, 2025

## 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>
   id?: string
   isDisabled?: boolean
   keyboardActivation?: 'automatic' | 'manual' = 'automatic'
-  labelBehavior?: 'show' | 'hide' = 'show'
   onSelectionChange?: (Key) => void
   orientation?: Orientation = 'horizontal'
   selectedKey?: Key | null
   slot?: string | null
 }

/@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 | (T) => ReactNode
+  children?: 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>
   id?: string
   isDisabled?: boolean
   keyboardActivation?: 'automatic' | 'manual' = 'automatic'
-  labelBehavior?: 'show' | 'hide' = 'show'
   onSelectionChange?: (Key) => void
   orientation?: Orientation = 'horizontal'
   selectedKey?: Key | null
   slot?: string | null
 }

/@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 | (T) => ReactNode
+  children?: ReactNode
   dependencies?: Array<any>
   items?: Iterable<T>
   styles?: StylesProp
 }

@snowystinger
Copy link
Member

We did this in v3 as well. Should we leave it as is and focus on changing the implementation to something that is acceptable?
I assume we talked to accessibility team about this and I'll catch up this week?

I was thinking of moving the picker to just outside the tablist as a sibling. I wasn't sure what to put in the tablist though, a visually hidden tab..?

@devongovett
Copy link
Member

@snowystinger it's different in v3. The picker was not nested inside the tablist, it was a sibling of it. Also the tabpanels in S2 still have aria-labelledby pointing at the tab that doesn't actually exist in the DOM. Instead they should point to the picker. This is a bit challenging to do with RAC (we overode things returned by hooks in v3). I think we should swap between RAC Tabs and a custom component instead. Started trying to implement that here but we can discuss more next week. #7600

@LFDanLu LFDanLu added this pull request to the merge queue Jan 13, 2025
Merged via the queue into main with commit 191df02 Jan 13, 2025
30 checks passed
@LFDanLu LFDanLu deleted the revert_tabs_collapse branch January 13, 2025 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants