feat(s2): update accordion api to allow sibling elements in disclosure header #7179

yihuiliao · 2024-10-11T21:13:52Z

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:

rspbot · 2024-10-11T21:48:43Z

Build successful! 🎉

reidbarber

API and styles look good.

reidbarber · 2024-10-11T22:31:38Z

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

+      style={UNSAFE_style}
+      className={(UNSAFE_className ?? '') + style({display: 'flex', alignItems: 'center', gap: 4})}
+      ref={domRef}>
+      {children}


An option for shorthand: if typeof children === 'string', we could automatically wrap children in a DisclosureTrigger, which would let you use our existing API as well.

No need to implement this now, we could discuss it later

yihuiliao · 2024-10-11T22:37:00Z

I'll update the chromatic stories as well once we settle on the API that we like

ktabors

This is great! Just a few observations.

Density compact seems to have a focus ring issue.

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

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

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

snowystinger · 2024-10-13T23:28:17Z

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

@@ -34,15 +36,19 @@ export const Example: Story = {
        <Accordion {...args}>
          <Disclosure id="files">
            <DisclosureHeader>
-              Files
+              <DisclosureTrigger>


This pattern is opposite to our other triggers
<DialogTrigger> is the outermost, whereas here, DisclosureTrigger is the innermost

That's because the header can contain elements that are not part of the trigger (eg adjacent buttons).

I understand that, but it also doesn't include the element that it opens like our other triggers
See my other concerns in my next comment down about level

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

yihuiliao · 2024-10-14T20:16:46Z

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

+  };
+
+  // maps to one size smaller in the compact density to ensure there is space between the top and bottom of the action button and container
+  // @ts-ignore


i'll take a look at this later, just wanted to get the lint working for testing

what was the error?

rspbot · 2024-10-14T20:26:18Z

Build successful! 🎉

devongovett · 2024-10-14T21:14:13Z

My only question is whether we want to have a shortcut so you don't need to write <DisclosureHeader><DisclosureTitle>Title</DisclosureTitle></DisclosureHeader> in the common case where there are no sibling buttons. We could potentially have just <DisclosureTitle> and automatically wrap in a header if it isn't already in one, similar to what we do with <Modal> and <ModalOverlay>. There may be other options as well. Anyone else have input?

snowystinger · 2024-10-14T21:48:56Z

Does it make more sense to have DisclosureTitle do the shortcut wrapping? It feels opposite to button and DisclosureHeader should wrap a string child in the DisclosureTitle automatically

devongovett · 2024-10-14T22:53:29Z

It feels opposite to button and DisclosureHeader should wrap a string child in the DisclosureTitle automatically

But then as soon as you put a <span> or <FormatMessage> or something it wouldn't work right?

snowystinger · 2024-10-14T23:16:07Z

It feels opposite to button and DisclosureHeader should wrap a string child in the DisclosureTitle automatically

But then as soon as you put a <span> or <FormatMessage> or something it wouldn't work right?

This is true. I guess I'm ok with it. I don't have a better idea.

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

snowystinger · 2024-10-14T23:24:00Z

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

+  };
+
+  // maps to one size smaller in the compact density to ensure there is space between the top and bottom of the action button and container
+  // @ts-ignore


what was the error?

rspbot · 2024-10-14T23:45:04Z

Build successful! 🎉

snowystinger

UPDATE seems to have been a cache issue, I can't reproduce in incognito

Seeing an error when I try to visit the storybook build https://reactspectrum.blob.core.windows.net/reactspectrum/844be69102b36467a58c5be2e1aaee9e90d50c48/storybook/index.html?path=/story/accordion--default&providerSwitcher-express=false

Not seeing the error on Main, but this only have changes for s2... so I'm not sure what has changed.

snowystinger

are we doing the auto wrapping in a followup?

yihuiliao · 2024-10-15T16:52:17Z

are we doing the auto wrapping in a followup?

i'll just do it in this pr. i'd like to get the api figured out so that i can update the chromatic stories too

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

rspbot · 2024-10-15T18:53:42Z

Build successful! 🎉

ktabors

Out of scope. Shouldn't there be an example with an expanded disclosure?

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

rspbot · 2024-10-15T23:26:22Z

Build successful! 🎉

rspbot · 2024-10-16T02:56:45Z

Build successful! 🎉

snowystinger

looks like a style got lost somewhere?
https://www.chromatic.com/build?appId=5f0dd5ad2b5fc10022a2e320&number=807

actually, looks like I can't interact with/open the disclosure anymore for some of these

yihuiliao · 2024-10-16T17:30:40Z

looks like a style got lost somewhere? https://www.chromatic.com/build?appId=5f0dd5ad2b5fc10022a2e320&number=807

ah good catch. i think that's because i forgot to update the chromatic stories to use DisclosureTitle.

actually, looks like I can't interact with/open the disclosure anymore for some of these

which one's specifically can you not interact with/open and which browser? im not able to reproduce

rspbot · 2024-10-16T17:40:37Z

Build successful! 🎉

rspbot · 2024-10-16T17:40:38Z

## API Changes

@react-spectrum/s2

/@react-spectrum/s2:DisclosureHeader

 DisclosureHeader {
   UNSAFE_className?: string
   UNSAFE_style?: CSSProperties
   children: React.ReactNode
   id?: string
-  level?: number = 3
 }

/@react-spectrum/s2:DisclosureTitle

+DisclosureTitle {
+  UNSAFE_className?: string
+  UNSAFE_style?: CSSProperties
+  children: React.ReactNode
+  id?: string
+  level?: number = 3
+}

snowystinger · 2024-10-16T21:02:17Z

This is the chromatic story where i couldn't open it https://5f0dd5ad2b5fc10022a2e320-ykjpggsppc.chromatic.com/?path=/story/s2-chromatic-disclosure--example which is explained by not using the right elements. It's fixed now with your updates.

yihuiliao added 2 commits October 11, 2024 14:13

update s2 accordion api

9e9d41b

fix lint

ecd2717

reidbarber previously approved these changes Oct 11, 2024

View reviewed changes

ktabors reviewed Oct 12, 2024

View reviewed changes

snowystinger reviewed Oct 13, 2024

View reviewed changes

rename to disclosure title, context for action button

ad19563

yihuiliao dismissed reidbarber’s stale review via ad19563 October 14, 2024 18:47

fix lint

b09f065

yihuiliao commented Oct 14, 2024

View reviewed changes

remove console log

98d31d6

snowystinger reviewed Oct 14, 2024

View reviewed changes

fix typscript

844be69

snowystinger reviewed Oct 15, 2024

View reviewed changes

snowystinger previously approved these changes Oct 15, 2024

View reviewed changes

ktabors reviewed Oct 15, 2024

View reviewed changes

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

yihuiliao added 2 commits October 15, 2024 11:21

allow disclosure title to wrap header

eae04b9

add chromatic stories

d2a023f

yihuiliao dismissed snowystinger’s stale review via d2a023f October 15, 2024 18:44

ktabors reviewed Oct 15, 2024

View reviewed changes

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

make code more concise

ceae83a

yihuiliao changed the title ~~Update S2 Accordion API~~ feat: update accordion api to allow sibling elements in disclosure header Oct 15, 2024

yihuiliao changed the title ~~feat: update accordion api to allow sibling elements in disclosure header~~ feat(s2): update accordion api to allow sibling elements in disclosure header Oct 15, 2024

fix lint

b090d1e

Merge branch 'main' into update-s2-accordion-api

cf36bbf

snowystinger reviewed Oct 16, 2024

View reviewed changes

fix chromatic stories

384a6de

snowystinger approved these changes Oct 16, 2024

View reviewed changes

reidbarber approved these changes Oct 16, 2024

View reviewed changes

yihuiliao added this pull request to the merge queue Oct 17, 2024

Merged via the queue into main with commit b958085 Oct 17, 2024
32 checks passed

yihuiliao deleted the update-s2-accordion-api branch October 17, 2024 18:13

feat(s2): update accordion api to allow sibling elements in disclosure header #7179

feat(s2): update accordion api to allow sibling elements in disclosure header #7179

Uh oh!

Conversation

yihuiliao commented Oct 11, 2024

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

Uh oh!

rspbot commented Oct 11, 2024

Uh oh!

reidbarber left a comment

Choose a reason for hiding this comment

Uh oh!

reidbarber Oct 11, 2024 • edited by snowystinger Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yihuiliao commented Oct 11, 2024

Uh oh!

ktabors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

snowystinger Oct 13, 2024

Choose a reason for hiding this comment

Uh oh!

devongovett Oct 14, 2024

Choose a reason for hiding this comment

Uh oh!

snowystinger Oct 14, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yihuiliao Oct 14, 2024

Choose a reason for hiding this comment

Uh oh!

snowystinger Oct 14, 2024

Choose a reason for hiding this comment

Uh oh!

rspbot commented Oct 14, 2024

Uh oh!

devongovett commented Oct 14, 2024

Uh oh!

snowystinger commented Oct 14, 2024

Uh oh!

devongovett commented Oct 14, 2024

Uh oh!

snowystinger commented Oct 14, 2024

Uh oh!

Uh oh!

snowystinger Oct 14, 2024

Choose a reason for hiding this comment

Uh oh!

rspbot commented Oct 14, 2024

Uh oh!

snowystinger left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snowystinger left a comment

Choose a reason for hiding this comment

Uh oh!

yihuiliao commented Oct 15, 2024

Uh oh!

Uh oh!

rspbot commented Oct 15, 2024

Uh oh!

ktabors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rspbot commented Oct 15, 2024

Uh oh!

rspbot commented Oct 16, 2024

Uh oh!

reidbarber Oct 11, 2024 •

edited by snowystinger

Loading

snowystinger left a comment •

edited

Loading

snowystinger left a comment •

edited

Loading

yihuiliao commented Oct 16, 2024 •

edited

Loading