Add accordion features #518
Conversation
🦋 Changeset detectedLatest commit: d346d76 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
cypress-design
|
||||||||||||||||||||||||||||||||||||||||
| Project |
cypress-design
|
| Branch Review |
karl/accordion-features
|
| Run status |
|
| Run duration | 05m 14s |
| Commit |
|
| Committer | Karl Snyder |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
1
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
230
|
| View all changes introduced in this branch ↗︎ | |
UI Coverage
10.87%
|
|
|---|---|
|
Untested elements
|
189
|
|
Tested elements
|
25
|
Accessibility
99.37%
|
|
|---|---|
|
Failed rules
|
0 critical
1 serious
0 moderate
1 minor
|
|
Failed elements
|
22
|
karlsnyder0
requested review from
emilyrohrbough,
estrada9166,
MuazOthman and
emilmilanov
| /** | ||
| * If true, prevents the accordion from toggling open or closed. | ||
| */ | ||
| locked?: boolean |
There was a problem hiding this comment.
Should this be "disabled"?
There was a problem hiding this comment.
No, it should be locked. Disabled is a different state that shows the component in a disabled state. This allows us to completely lock the component from bring opened.
| onToggle = () => {}, | ||
| onToggleBlocked = () => {}, |
There was a problem hiding this comment.
I'm not sure we should be setting default empty funcs for these props
There was a problem hiding this comment.
They are optional. Are you recommending that we allow them to be undefined and check for that instead of calling the empty fn?
There was a problem hiding this comment.
ok, I'll make that change.
| @@ -101,16 +102,86 @@ const props = defineProps<{ | |||
| * removes the content wrapper from the content. | |||
| */ | |||
| fullWidthContent?: boolean | |||
| open?: boolean | |||
There was a problem hiding this comment.
isOpen describes this is a boolean
No. The locked state works with components inside the summary. There's no obvious UI indicators. |
| onClickRet = onClickSummary(event) | ||
| } | ||
|
|
||
| // TODO: Clone the original event then check propagation. |
There was a problem hiding this comment.
It is. The ideal code would clone the original event and link stopPropagation. For now, that's not ideal for the time we have to finish, we'll keep it simple for now and it's not a blocker.
There was a problem hiding this comment.
is there an issue to link to track that?
There was a problem hiding this comment.
No. Actually I'm not sure we need it until we know that we will need it. In other words, it's not absolutely required but I wanted to leave a note in there in case someone wonders why stopPropagation doesn't work.
| * Callback triggered when the accordion toggles open or closed. | ||
| * @param open - The new open state of the accordion. | ||
| */ | ||
| onToggle?: (open: boolean) => void |
There was a problem hiding this comment.
missing open from prop types
There was a problem hiding this comment.
see- React.HTMLProps<HTMLDetailsElement>. It's not missing.
Co-authored-by: Emily Wisniewski (Rohrbough) <[email protected]>
IMPORTANT! Cancelable toggle is not supported with browser native details elements. Event listener capture must be used to intercept the event early enough.