Skip to content

Conversation

@reidbarber
Copy link
Member

@reidbarber reidbarber commented Oct 21, 2024

From testing:

  • Remove UNSTABLE_ prefix
  • useDisclosure: add aria-hidden if disclosure is collapsed (avoid dup announcement in Talkback and VO)
  • S2 Accoridon: add controlled example and code to storybook
  • S2 Accordion: add active state from designs
  • v3 Accordion: Add hover state when open
  • v3 Accordion: Make non-quiet corners not rounded unless focused (DG)
  • v3 Accordion: HCM needs borders removed
  • v3 Accordion: fix Command+F not working in v3 accordion (remove display: none)

From review:

  • v3 Accordion: add background color change for keyboard focus and pressed state
  • Ignore repeat keydown events

From standup:

  • Switch keyboard trigger to use onPressStart
  • Add style props to v3 Disclosure, DisclosurePanel, and DisclosureHeader
  • Fix bottom border on S2 Disclosure when it has sibling elements and isn't in an Accordion
  • Fix bottom border on v3 Disclosure when it has sibling elements and isn't in an Accordion

Docs changes:

  • Disclosure docs: Add render props table to DisclosurePanel in DisclosureGroup
  • Disclosure docs: Add example to docs showing a button adjacent to heading
  • Disclosure docs: mention hidden=until-found in features section like hook docs. Also mention that a disclosure can be built with the HTML <details> and <summary> elements but there are styling limitations with things like adjacent buttons
  • Disclosure docs: Disclosure group example didn't render? Link
  • v3 Accordion docs: Disclosure, DisclosurePanel, DisclosureHeading should have prop tables if they have props that users can apply
  • Disclosure docs: Mention that expandedKeys must match the id prop of the Disclosure component
  • Disclosure docs: Fix anatomy svg size on mobile
  • useDisclosure docs: When tapping on the disclosure headers, an outline around the header appears.

✅ 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 Oct 21, 2024

@rspbot
Copy link

rspbot commented Oct 22, 2024

snowystinger
snowystinger previously approved these changes Oct 22, 2024
Copy link
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick on the test, otherwise looks good
tested in chrome

@rspbot
Copy link

rspbot commented Oct 22, 2024

snowystinger
snowystinger previously approved these changes Oct 22, 2024
@rspbot
Copy link

rspbot commented Oct 22, 2024

@rspbot
Copy link

rspbot commented Oct 22, 2024

@rspbot
Copy link

rspbot commented Oct 23, 2024

Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on Android and desktop, just two small comments

@rspbot
Copy link

rspbot commented Oct 23, 2024

snowystinger
snowystinger previously approved these changes Oct 23, 2024
* docs: follow-up on accordion docs from testing

* updates svgs for mobile, follow up from reviews

* prevent outline on header on android

* fix examples in useDisclosure on mobile

* update focus visible

* add focusProps to disclosure group example

* use merge props
@rspbot
Copy link

rspbot commented Oct 23, 2024

@rspbot
Copy link

rspbot commented Oct 23, 2024

## API Changes

react-aria-components

/react-aria-components:UNSTABLE_Disclosure

-UNSTABLE_Disclosure {
-  children?: ReactNode | ((DisclosureRenderProps & {
-    defaultChildren: ReactNode | undefined
-})) => ReactNode
-  className?: string | ((DisclosureRenderProps & {
-    defaultClassName: string | undefined
-})) => string
-  defaultExpanded?: boolean
-  id?: Key
-  isDisabled?: boolean
-  isExpanded?: boolean
-  onExpandedChange?: (boolean) => void
-  slot?: string | null
-  style?: CSSProperties | ((DisclosureRenderProps & {
-    defaultStyle: CSSProperties
-})) => CSSProperties | undefined
-}

/react-aria-components:UNSTABLE_DisclosureGroup

-UNSTABLE_DisclosureGroup {
-  allowsMultipleExpanded?: boolean
-  children?: ReactNode | ((DisclosureGroupRenderProps & {
-    defaultChildren: ReactNode | undefined
-})) => ReactNode
-  className?: string | ((DisclosureGroupRenderProps & {
-    defaultClassName: string | undefined
-})) => string
-  defaultExpandedKeys?: Iterable<Key>
-  expandedKeys?: Iterable<Key>
-  id?: string
-  isDisabled?: boolean
-  onExpandedChange?: (Set<Key>) => any
-  style?: CSSProperties | ((DisclosureGroupRenderProps & {
-    defaultStyle: CSSProperties
-})) => CSSProperties | undefined
-}

/react-aria-components:UNSTABLE_DisclosurePanel

-UNSTABLE_DisclosurePanel {
-  children: ReactNode
-  className?: string | ((DisclosurePanelRenderProps & {
-    defaultClassName: string | undefined
-})) => string
-  id?: string
-  role?: 'group' | 'region' = 'group'
-  style?: CSSProperties | ((DisclosurePanelRenderProps & {
-    defaultStyle: CSSProperties
-})) => CSSProperties | undefined
-}

/react-aria-components:Disclosure

+Disclosure {
+  children?: ReactNode | ((DisclosureRenderProps & {
+    defaultChildren: ReactNode | undefined
+})) => ReactNode
+  className?: string | ((DisclosureRenderProps & {
+    defaultClassName: string | undefined
+})) => string
+  defaultExpanded?: boolean
+  id?: Key
+  isDisabled?: boolean
+  isExpanded?: boolean
+  onExpandedChange?: (boolean) => void
+  slot?: string | null
+  style?: CSSProperties | ((DisclosureRenderProps & {
+    defaultStyle: CSSProperties
+})) => CSSProperties | undefined
+}

/react-aria-components:DisclosureGroup

+DisclosureGroup {
+  allowsMultipleExpanded?: boolean
+  children?: ReactNode | ((DisclosureGroupRenderProps & {
+    defaultChildren: ReactNode | undefined
+})) => ReactNode
+  className?: string | ((DisclosureGroupRenderProps & {
+    defaultClassName: string | undefined
+})) => string
+  defaultExpandedKeys?: Iterable<Key>
+  expandedKeys?: Iterable<Key>
+  id?: string
+  isDisabled?: boolean
+  onExpandedChange?: (Set<Key>) => any
+  style?: CSSProperties | ((DisclosureGroupRenderProps & {
+    defaultStyle: CSSProperties
+})) => CSSProperties | undefined
+}

/react-aria-components:DisclosurePanel

+DisclosurePanel {
+  children: ReactNode
+  className?: string | ((DisclosurePanelRenderProps & {
+    defaultClassName: string | undefined
+})) => string
+  id?: string
+  role?: 'group' | 'region' = 'group'
+  style?: CSSProperties | ((DisclosurePanelRenderProps & {
+    defaultStyle: CSSProperties
+})) => CSSProperties | undefined
+}

@react-spectrum/accordion

/@react-spectrum/accordion:Disclosure

 Disclosure {
+  UNSAFE_className?: string
+  UNSAFE_style?: CSSProperties
+  alignSelf?: Responsive<'auto' | 'normal' | 'start' | 'end' | 'center' | 'flex-start' | 'flex-end' | 'self-start' | 'self-end' | 'stretch'>
   aria-describedby?: string
   aria-details?: string
   aria-label?: string
   aria-labelledby?: string
+  bottom?: Responsive<DimensionValue>
   children: React.ReactNode
   defaultExpanded?: boolean
+  end?: Responsive<DimensionValue>
+  flex?: Responsive<string | number | boolean>
+  flexBasis?: Responsive<number | string>
+  flexGrow?: Responsive<number>
+  flexShrink?: Responsive<number>
+  gridArea?: Responsive<string>
+  gridColumn?: Responsive<string>
+  gridColumnEnd?: Responsive<string>
+  gridColumnStart?: Responsive<string>
+  gridRow?: Responsive<string>
+  gridRowEnd?: Responsive<string>
+  gridRowStart?: Responsive<string>
+  height?: Responsive<DimensionValue>
   id?: Key
   isDisabled?: boolean
   isExpanded?: boolean
+  isHidden?: Responsive<boolean>
   isQuiet?: boolean
+  justifySelf?: Responsive<'auto' | 'normal' | 'start' | 'end' | 'flex-start' | 'flex-end' | 'self-start' | 'self-end' | 'center' | 'left' | 'right' | 'stretch'>
+  left?: Responsive<DimensionValue>
+  margin?: Responsive<DimensionValue>
+  marginBottom?: Responsive<DimensionValue>
+  marginEnd?: Responsive<DimensionValue>
+  marginStart?: Responsive<DimensionValue>
+  marginTop?: Responsive<DimensionValue>
+  marginX?: Responsive<DimensionValue>
+  marginY?: Responsive<DimensionValue>
+  maxHeight?: Responsive<DimensionValue>
+  maxWidth?: Responsive<DimensionValue>
+  minHeight?: Responsive<DimensionValue>
+  minWidth?: Responsive<DimensionValue>
   onExpandedChange?: (boolean) => void
+  order?: Responsive<number>
+  position?: Responsive<'static' | 'relative' | 'absolute' | 'fixed' | 'sticky'>
+  right?: Responsive<DimensionValue>
   slot?: string | null
+  start?: Responsive<DimensionValue>
+  top?: Responsive<DimensionValue>
+  width?: Responsive<DimensionValue>
+  zIndex?: Responsive<number>
 }

/@react-spectrum/accordion:DisclosureHeader

 DisclosureHeader {
+  UNSAFE_className?: string
+  UNSAFE_style?: CSSProperties
+  alignSelf?: Responsive<'auto' | 'normal' | 'start' | 'end' | 'center' | 'flex-start' | 'flex-end' | 'self-start' | 'self-end' | 'stretch'>
   aria-describedby?: string
   aria-details?: string
   aria-label?: string
   aria-labelledby?: string
+  bottom?: Responsive<DimensionValue>
   children: React.ReactNode
+  end?: Responsive<DimensionValue>
+  flex?: Responsive<string | number | boolean>
+  flexBasis?: Responsive<number | string>
+  flexGrow?: Responsive<number>
+  flexShrink?: Responsive<number>
+  gridArea?: Responsive<string>
+  gridColumn?: Responsive<string>
+  gridColumnEnd?: Responsive<string>
+  gridColumnStart?: Responsive<string>
+  gridRow?: Responsive<string>
+  gridRowEnd?: Responsive<string>
+  gridRowStart?: Responsive<string>
+  height?: Responsive<DimensionValue>
   id?: string
+  isHidden?: Responsive<boolean>
+  justifySelf?: Responsive<'auto' | 'normal' | 'start' | 'end' | 'flex-start' | 'flex-end' | 'self-start' | 'self-end' | 'center' | 'left' | 'right' | 'stretch'>
+  left?: Responsive<DimensionValue>
   level?: number = 3
+  margin?: Responsive<DimensionValue>
+  marginBottom?: Responsive<DimensionValue>
+  marginEnd?: Responsive<DimensionValue>
+  marginStart?: Responsive<DimensionValue>
+  marginTop?: Responsive<DimensionValue>
+  marginX?: Responsive<DimensionValue>
+  marginY?: Responsive<DimensionValue>
+  maxHeight?: Responsive<DimensionValue>
+  maxWidth?: Responsive<DimensionValue>
+  minHeight?: Responsive<DimensionValue>
+  minWidth?: Responsive<DimensionValue>
+  order?: Responsive<number>
+  position?: Responsive<'static' | 'relative' | 'absolute' | 'fixed' | 'sticky'>
+  right?: Responsive<DimensionValue>
+  start?: Responsive<DimensionValue>
+  top?: Responsive<DimensionValue>
+  width?: Responsive<DimensionValue>
+  zIndex?: Responsive<number>
 }

/@react-spectrum/accordion:DisclosurePanel

 DisclosurePanel {
+  UNSAFE_className?: string
+  UNSAFE_style?: CSSProperties
+  alignSelf?: Responsive<'auto' | 'normal' | 'start' | 'end' | 'center' | 'flex-start' | 'flex-end' | 'self-start' | 'self-end' | 'stretch'>
   aria-describedby?: string
   aria-details?: string
   aria-label?: string
   aria-labelledby?: string
+  bottom?: Responsive<DimensionValue>
   children: React.ReactNode
+  end?: Responsive<DimensionValue>
+  flex?: Responsive<string | number | boolean>
+  flexBasis?: Responsive<number | string>
+  flexGrow?: Responsive<number>
+  flexShrink?: Responsive<number>
+  gridArea?: Responsive<string>
+  gridColumn?: Responsive<string>
+  gridColumnEnd?: Responsive<string>
+  gridColumnStart?: Responsive<string>
+  gridRow?: Responsive<string>
+  gridRowEnd?: Responsive<string>
+  gridRowStart?: Responsive<string>
+  height?: Responsive<DimensionValue>
   id?: string
+  isHidden?: Responsive<boolean>
+  justifySelf?: Responsive<'auto' | 'normal' | 'start' | 'end' | 'flex-start' | 'flex-end' | 'self-start' | 'self-end' | 'center' | 'left' | 'right' | 'stretch'>
+  left?: Responsive<DimensionValue>
+  margin?: Responsive<DimensionValue>
+  marginBottom?: Responsive<DimensionValue>
+  marginEnd?: Responsive<DimensionValue>
+  marginStart?: Responsive<DimensionValue>
+  marginTop?: Responsive<DimensionValue>
+  marginX?: Responsive<DimensionValue>
+  marginY?: Responsive<DimensionValue>
+  maxHeight?: Responsive<DimensionValue>
+  maxWidth?: Responsive<DimensionValue>
+  minHeight?: Responsive<DimensionValue>
+  minWidth?: Responsive<DimensionValue>
+  order?: Responsive<number>
+  position?: Responsive<'static' | 'relative' | 'absolute' | 'fixed' | 'sticky'>
+  right?: Responsive<DimensionValue>
   role?: 'group' | 'region' = 'group'
+  start?: Responsive<DimensionValue>
+  top?: Responsive<DimensionValue>
+  width?: Responsive<DimensionValue>
+  zIndex?: Responsive<number>
 }

/@react-spectrum/accordion:SpectrumDisclosureProps

 SpectrumDisclosureProps {
+  UNSAFE_className?: string
+  UNSAFE_style?: CSSProperties
+  alignSelf?: Responsive<'auto' | 'normal' | 'start' | 'end' | 'center' | 'flex-start' | 'flex-end' | 'self-start' | 'self-end' | 'stretch'>
   aria-describedby?: string
   aria-details?: string
   aria-label?: string
   aria-labelledby?: string
+  bottom?: Responsive<DimensionValue>
   children: React.ReactNode
   defaultExpanded?: boolean
+  end?: Responsive<DimensionValue>
+  flex?: Responsive<string | number | boolean>
+  flexBasis?: Responsive<number | string>
+  flexGrow?: Responsive<number>
+  flexShrink?: Responsive<number>
+  gridArea?: Responsive<string>
+  gridColumn?: Responsive<string>
+  gridColumnEnd?: Responsive<string>
+  gridColumnStart?: Responsive<string>
+  gridRow?: Responsive<string>
+  gridRowEnd?: Responsive<string>
+  gridRowStart?: Responsive<string>
+  height?: Responsive<DimensionValue>
   id?: Key
   isDisabled?: boolean
   isExpanded?: boolean
+  isHidden?: Responsive<boolean>
   isQuiet?: boolean
+  justifySelf?: Responsive<'auto' | 'normal' | 'start' | 'end' | 'flex-start' | 'flex-end' | 'self-start' | 'self-end' | 'center' | 'left' | 'right' | 'stretch'>
+  left?: Responsive<DimensionValue>
+  margin?: Responsive<DimensionValue>
+  marginBottom?: Responsive<DimensionValue>
+  marginEnd?: Responsive<DimensionValue>
+  marginStart?: Responsive<DimensionValue>
+  marginTop?: Responsive<DimensionValue>
+  marginX?: Responsive<DimensionValue>
+  marginY?: Responsive<DimensionValue>
+  maxHeight?: Responsive<DimensionValue>
+  maxWidth?: Responsive<DimensionValue>
+  minHeight?: Responsive<DimensionValue>
+  minWidth?: Responsive<DimensionValue>
   onExpandedChange?: (boolean) => void
+  order?: Responsive<number>
+  position?: Responsive<'static' | 'relative' | 'absolute' | 'fixed' | 'sticky'>
+  right?: Responsive<DimensionValue>
   slot?: string | null
+  start?: Responsive<DimensionValue>
+  top?: Responsive<DimensionValue>
+  width?: Responsive<DimensionValue>
+  zIndex?: Responsive<number>
 }

/@react-spectrum/accordion:SpectrumDisclosurePanelProps

 SpectrumDisclosurePanelProps {
+  UNSAFE_className?: string
+  UNSAFE_style?: CSSProperties
+  alignSelf?: Responsive<'auto' | 'normal' | 'start' | 'end' | 'center' | 'flex-start' | 'flex-end' | 'self-start' | 'self-end' | 'stretch'>
   aria-describedby?: string
   aria-details?: string
   aria-label?: string
   aria-labelledby?: string
+  bottom?: Responsive<DimensionValue>
   children: React.ReactNode
+  end?: Responsive<DimensionValue>
+  flex?: Responsive<string | number | boolean>
+  flexBasis?: Responsive<number | string>
+  flexGrow?: Responsive<number>
+  flexShrink?: Responsive<number>
+  gridArea?: Responsive<string>
+  gridColumn?: Responsive<string>
+  gridColumnEnd?: Responsive<string>
+  gridColumnStart?: Responsive<string>
+  gridRow?: Responsive<string>
+  gridRowEnd?: Responsive<string>
+  gridRowStart?: Responsive<string>
+  height?: Responsive<DimensionValue>
   id?: string
+  isHidden?: Responsive<boolean>
+  justifySelf?: Responsive<'auto' | 'normal' | 'start' | 'end' | 'flex-start' | 'flex-end' | 'self-start' | 'self-end' | 'center' | 'left' | 'right' | 'stretch'>
+  left?: Responsive<DimensionValue>
+  margin?: Responsive<DimensionValue>
+  marginBottom?: Responsive<DimensionValue>
+  marginEnd?: Responsive<DimensionValue>
+  marginStart?: Responsive<DimensionValue>
+  marginTop?: Responsive<DimensionValue>
+  marginX?: Responsive<DimensionValue>
+  marginY?: Responsive<DimensionValue>
+  maxHeight?: Responsive<DimensionValue>
+  maxWidth?: Responsive<DimensionValue>
+  minHeight?: Responsive<DimensionValue>
+  minWidth?: Responsive<DimensionValue>
+  order?: Responsive<number>
+  position?: Responsive<'static' | 'relative' | 'absolute' | 'fixed' | 'sticky'>
+  right?: Responsive<DimensionValue>
   role?: 'group' | 'region' = 'group'
+  start?: Responsive<DimensionValue>
+  top?: Responsive<DimensionValue>
+  width?: Responsive<DimensionValue>
+  zIndex?: Responsive<number>
 }

/@react-spectrum/accordion:SpectrumDisclosureHeaderProps

 SpectrumDisclosureHeaderProps {
+  UNSAFE_className?: string
+  UNSAFE_style?: CSSProperties
+  alignSelf?: Responsive<'auto' | 'normal' | 'start' | 'end' | 'center' | 'flex-start' | 'flex-end' | 'self-start' | 'self-end' | 'stretch'>
   aria-describedby?: string
   aria-details?: string
   aria-label?: string
   aria-labelledby?: string
+  bottom?: Responsive<DimensionValue>
   children: React.ReactNode
+  end?: Responsive<DimensionValue>
+  flex?: Responsive<string | number | boolean>
+  flexBasis?: Responsive<number | string>
+  flexGrow?: Responsive<number>
+  flexShrink?: Responsive<number>
+  gridArea?: Responsive<string>
+  gridColumn?: Responsive<string>
+  gridColumnEnd?: Responsive<string>
+  gridColumnStart?: Responsive<string>
+  gridRow?: Responsive<string>
+  gridRowEnd?: Responsive<string>
+  gridRowStart?: Responsive<string>
+  height?: Responsive<DimensionValue>
   id?: string
+  isHidden?: Responsive<boolean>
+  justifySelf?: Responsive<'auto' | 'normal' | 'start' | 'end' | 'flex-start' | 'flex-end' | 'self-start' | 'self-end' | 'center' | 'left' | 'right' | 'stretch'>
+  left?: Responsive<DimensionValue>
   level?: number = 3
+  margin?: Responsive<DimensionValue>
+  marginBottom?: Responsive<DimensionValue>
+  marginEnd?: Responsive<DimensionValue>
+  marginStart?: Responsive<DimensionValue>
+  marginTop?: Responsive<DimensionValue>
+  marginX?: Responsive<DimensionValue>
+  marginY?: Responsive<DimensionValue>
+  maxHeight?: Responsive<DimensionValue>
+  maxWidth?: Responsive<DimensionValue>
+  minHeight?: Responsive<DimensionValue>
+  minWidth?: Responsive<DimensionValue>
+  order?: Responsive<number>
+  position?: Responsive<'static' | 'relative' | 'absolute' | 'fixed' | 'sticky'>
+  right?: Responsive<DimensionValue>
+  start?: Responsive<DimensionValue>
+  top?: Responsive<DimensionValue>
+  width?: Responsive<DimensionValue>
+  zIndex?: Responsive<number>
 }

Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small docs typos but approving for testing if you want to handle them in a follow up



Custom styled disclosure groups can be difficult to implement in an accessible way with the correct ARIA pattern. When implementing, you must manually ensure that the button and panel are semantically connected via ids for accessibility. `DisclosureGroup` helps with this and other accessibility features while allowing for custom styling.
Accordions can be built by combing multiple disclosures built in HTML with the [&lt;details&gt;](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/details) and [&lt;summary&gt;](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/summary) HTML element, but this can be difficult to style especially when adding adjacent interactive elements, like buttons, to the disclosure's heading. `Accordion` helps achieve accessible disclosures implemented with the correct ARIA pattern while making custom styling easy.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think this is supposed to reference 'DisclosureGroup' instead of accordion

```
## Props

### Disclosure
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
### Disclosure
### Accordion

import docs from 'docs:@react-spectrum/accordion';
import {HeaderInfo, PropTable, TypeLink, PageDescription} from '@react-spectrum/docs';
import packageData from '@react-spectrum/accordion/package.json';
import ChevronRight from '@spectrum-icons/workflow/ChevronRight';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh oops that's leftover from trying something else, i can follow-up on it

Copy link
Member

@yihuiliao yihuiliao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approving for testing. i can make a follow-up for the docs stuff that Daniel commented about

@yihuiliao yihuiliao added this pull request to the merge queue Oct 28, 2024
Merged via the queue into main with commit 6607108 Oct 28, 2024
32 checks passed
@yihuiliao yihuiliao deleted the disclosure-release-testing branch October 28, 2024 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants