feat(Disclosure): add CollapsedDetails property - #2755
Open
GermanVor wants to merge 2 commits into
Open
Conversation
Contributor
Reviewer's GuideAdds a new Disclosure.CollapsedDetails subcomponent that renders overlay content only while the disclosure is collapsed, wires it into the Disclosure composition/children parsing, and provides styling, tests, docs, and stories for its behavior and API. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- Wrapping
detailsContentinside the newbodydiv only whencollapsedDetailsContentis present changes the DOM structure conditionally; consider always rendering the same wrapper to avoid layout/selector differences based on whether collapsed details are used. - When the disclosure is expanded,
Disclosure.CollapsedDetailsis only hidden via CSS andaria-hidden, but its children may still be focusable and reachable via keyboard; consider preventing focus (e.g., managing tabIndex or pointer-events) whenvisibleis false to align visual and accessibility behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Wrapping `detailsContent` inside the new `body` div only when `collapsedDetailsContent` is present changes the DOM structure conditionally; consider always rendering the same wrapper to avoid layout/selector differences based on whether collapsed details are used.
- When the disclosure is expanded, `Disclosure.CollapsedDetails` is only hidden via CSS and `aria-hidden`, but its children may still be focusable and reachable via keyboard; consider preventing focus (e.g., managing tabIndex or pointer-events) when `visible` is false to align visual and accessibility behavior.
## Individual Comments
### Comment 1
<location path="src/components/Disclosure/DisclosureCollapsedDetails/DisclosureCollapsedDetails.tsx" line_range="30" />
<code_context>
+ <CSSTransition
+ nodeRef={containerRef}
+ in={visible}
+ addEndListener={(done) => containerRef.current?.addEventListener('animationend', done)}
+ classNames={getCSSTransitionClassNames(b)}
+ appear={true}
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid accumulating multiple `animationend` listeners across transitions.
`addEndListener` currently attaches a new `animationend` handler on every transition and never removes the old ones, so the same node can end up with multiple listeners and `done` can fire multiple times.
Consider either:
- Using a one-time listener (e.g. `{ once: true }` if available), or
- Removing the listener via `removeEventListener` inside the callback, or
- Using `timeout` with `onEntered`/`onExited` if the animation event isn’t strictly required.
This keeps one effective listener per transition and avoids repeated registrations on the same node.
</issue_to_address>
### Comment 2
<location path="src/components/Disclosure/__tests__/Disclosure.test.tsx" line_range="207-216" />
<code_context>
expect(disclosure).toHaveClass('g-disclosure__trigger_arrow_end');
});
+ test('collapsed details are visible when not expanded', () => {
+ const collapsed = 'Collapsed content';
+ const details = 'Details content';
+ render(
+ <Disclosure expanded={false}>
+ <Disclosure.CollapsedDetails>{collapsed}</Disclosure.CollapsedDetails>
+ {details}
+ </Disclosure>,
+ );
+
+ expect(screen.getByText(collapsed)).toHaveClass('g-disclosure__collapsed-content_visible');
+ expect(screen.getByText(details)).not.toHaveClass('g-disclosure__content_visible');
+ });
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider asserting `aria-hidden` state for collapsed details to capture the accessibility behavior.
These tests only assert visibility via CSS classes, but `DisclosureCollapsedDetails` also toggles `aria-hidden` based on `expanded`. Please add `aria-hidden` checks for both collapsed and expanded cases so the tests cover the accessibility behavior and guard against regressions impacting screen readers.
Suggested implementation:
```typescript
test('collapsed details are visible when not expanded', () => {
const collapsed = 'Collapsed content';
const details = 'Details content';
render(
<Disclosure expanded={false}>
<Disclosure.CollapsedDetails>{collapsed}</Disclosure.CollapsedDetails>
{details}
</Disclosure>,
);
const collapsedElement = screen.getByText(collapsed);
const detailsElement = screen.getByText(details);
expect(collapsedElement).toHaveClass('g-disclosure__collapsed-content_visible');
expect(detailsElement).not.toHaveClass('g-disclosure__content_visible');
// Accessibility: collapsed details should not be hidden from assistive technologies when not expanded
expect(collapsedElement).toHaveAttribute('aria-hidden', 'false');
});
test('collapsed details are hidden from assistive technologies when expanded', () => {
const collapsed = 'Collapsed content';
const details = 'Details content';
render(
<Disclosure expanded={true}>
<Disclosure.CollapsedDetails>{collapsed}</Disclosure.CollapsedDetails>
{details}
</Disclosure>,
);
const collapsedElement = screen.getByText(collapsed);
// Accessibility: collapsed details should be hidden from assistive technologies when expanded
expect(collapsedElement).toHaveAttribute('aria-hidden', 'true');
});
```
If `aria-hidden` is applied on a different element (e.g. a wrapper around the collapsed content) rather than directly on the text node, you may need to adjust `collapsedElement` to query that specific element (for example, using `getByTestId` or traversing `collapsedElement.parentElement`) so the expectations match the actual DOM structure.
</issue_to_address>
### Comment 3
<location path="src/components/Disclosure/__tests__/Disclosure.test.tsx" line_range="237-246" />
<code_context>
+ expect(screen.getByText(details)).toHaveClass('g-disclosure__content_visible');
+ });
+
+ test('collapsed details visibility toggles when clicked', async () => {
+ const user = userEvent.setup();
+ const collapsed = 'Collapsed content';
+ render(
+ <Disclosure>
+ <Disclosure.CollapsedDetails>{collapsed}</Disclosure.CollapsedDetails>
+ </Disclosure>,
+ );
+ const disclosure = screen.getByRole('button');
+ const component = screen.getByText(collapsed);
+
+ expect(component).toHaveClass('g-disclosure__collapsed-content_visible');
+ await user.click(disclosure);
+ expect(component).not.toHaveClass('g-disclosure__collapsed-content_visible');
+ });
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test to verify `data-qa` behavior for `Disclosure.CollapsedDetails` (default and custom).
Please add tests that assert the default `data-qa` value (`DisclosureQa.COLLAPSED_DETAILS`) and that a custom `qa` prop overrides it. This will keep `Disclosure.CollapsedDetails` consistent with existing QA-related tests and provide coverage for the new QA hook.
Suggested implementation:
```typescript
expect(screen.getByText(collapsed)).not.toHaveClass(
'g-disclosure__collapsed-content_visible',
);
expect(screen.getByText(details)).toHaveClass('g-disclosure__content_visible');
});
test('collapsed details are visible when not expanded', () => {
const collapsed = 'Collapsed content';
const details = 'Details content';
render(
<Disclosure expanded={false}>
<Disclosure.CollapsedDetails>{collapsed}</Disclosure.CollapsedDetails>
{details}
</Disclosure>,
);
expect(screen.getByText(collapsed)).toHaveClass('g-disclosure__collapsed-content_visible');
});
test('collapsed details have default data-qa', () => {
const collapsed = 'Collapsed content';
render(
<Disclosure>
<Disclosure.CollapsedDetails>{collapsed}</Disclosure.CollapsedDetails>
</Disclosure>,
);
const component = screen.getByText(collapsed);
expect(component).toHaveAttribute('data-qa', DisclosureQa.COLLAPSED_DETAILS);
});
test('collapsed details allow overriding data-qa via qa prop', () => {
const collapsed = 'Collapsed content';
const customQa = 'custom-collapsed-details-qa';
render(
<Disclosure>
<Disclosure.CollapsedDetails qa={customQa}>{collapsed}</Disclosure.CollapsedDetails>
</Disclosure>,
);
const component = screen.getByText(collapsed);
expect(component).toHaveAttribute('data-qa', customQa);
});
```
1. At the top of `Disclosure.test.tsx`, import `DisclosureQa` from the same module used by other QA-related tests in this file. For example, if other tests already use QA enums, mirror that import style:
- `import { DisclosureQa } from '../Disclosure.qa';` (or the correct relative path in your codebase).
2. Ensure `Disclosure.CollapsedDetails` accepts a `qa` prop and passes it through to the rendered DOM element as `data-qa`, defaulting to `DisclosureQa.COLLAPSED_DETAILS` when `qa` is not provided. This behavior should already exist or be added in the `Disclosure.CollapsedDetails` implementation to make these tests pass.
</issue_to_address>
### Comment 4
<location path="src/components/Disclosure/README-ru.md" line_range="211-212" />
<code_context>
+
+Используйте компонент `Disclosure.CollapsedDetails`, чтобы отрендерить контент, который отображается
+только пока `Disclosure` свёрнут (`expanded === false`) и плавно исчезает при раскрытии. Он
+рендерится как абсолютно позиционированный оверлей поверх области с деталями, поэтому не увеличивает
+высоту свёрнутого компонента, и контент не «прыгает» при переключении.
+
+<!--SANDBOX
</code_context>
<issue_to_address>
**nitpick (typo):** Запятая перед «и контент» здесь, вероятно, лишняя для общего подлежащего.
Предлагаю убрать запятую и использовать формулировку: «…поэтому не увеличивает высоту свёрнутого компонента и контент не „прыгает“ при переключении».
```suggestion
рендерится как абсолютно позиционированный оверлей поверх области с деталями, поэтому не увеличивает
высоту свёрнутого компонента и контент не «прыгает» при переключении.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+207
to
+216
| test('collapsed details are visible when not expanded', () => { | ||
| const collapsed = 'Collapsed content'; | ||
| const details = 'Details content'; | ||
| render( | ||
| <Disclosure expanded={false}> | ||
| <Disclosure.CollapsedDetails>{collapsed}</Disclosure.CollapsedDetails> | ||
| {details} | ||
| </Disclosure>, | ||
| ); | ||
|
|
Contributor
There was a problem hiding this comment.
suggestion (testing): Consider asserting aria-hidden state for collapsed details to capture the accessibility behavior.
These tests only assert visibility via CSS classes, but DisclosureCollapsedDetails also toggles aria-hidden based on expanded. Please add aria-hidden checks for both collapsed and expanded cases so the tests cover the accessibility behavior and guard against regressions impacting screen readers.
Suggested implementation:
test('collapsed details are visible when not expanded', () => {
const collapsed = 'Collapsed content';
const details = 'Details content';
render(
<Disclosure expanded={false}>
<Disclosure.CollapsedDetails>{collapsed}</Disclosure.CollapsedDetails>
{details}
</Disclosure>,
);
const collapsedElement = screen.getByText(collapsed);
const detailsElement = screen.getByText(details);
expect(collapsedElement).toHaveClass('g-disclosure__collapsed-content_visible');
expect(detailsElement).not.toHaveClass('g-disclosure__content_visible');
// Accessibility: collapsed details should not be hidden from assistive technologies when not expanded
expect(collapsedElement).toHaveAttribute('aria-hidden', 'false');
});
test('collapsed details are hidden from assistive technologies when expanded', () => {
const collapsed = 'Collapsed content';
const details = 'Details content';
render(
<Disclosure expanded={true}>
<Disclosure.CollapsedDetails>{collapsed}</Disclosure.CollapsedDetails>
{details}
</Disclosure>,
);
const collapsedElement = screen.getByText(collapsed);
// Accessibility: collapsed details should be hidden from assistive technologies when expanded
expect(collapsedElement).toHaveAttribute('aria-hidden', 'true');
});If aria-hidden is applied on a different element (e.g. a wrapper around the collapsed content) rather than directly on the text node, you may need to adjust collapsedElement to query that specific element (for example, using getByTestId or traversing collapsedElement.parentElement) so the expectations match the actual DOM structure.
Comment on lines
+237
to
+246
| test('collapsed details visibility toggles when clicked', async () => { | ||
| const user = userEvent.setup(); | ||
| const collapsed = 'Collapsed content'; | ||
| render( | ||
| <Disclosure> | ||
| <Disclosure.CollapsedDetails>{collapsed}</Disclosure.CollapsedDetails> | ||
| </Disclosure>, | ||
| ); | ||
| const disclosure = screen.getByRole('button'); | ||
| const component = screen.getByText(collapsed); |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add a test to verify data-qa behavior for Disclosure.CollapsedDetails (default and custom).
Please add tests that assert the default data-qa value (DisclosureQa.COLLAPSED_DETAILS) and that a custom qa prop overrides it. This will keep Disclosure.CollapsedDetails consistent with existing QA-related tests and provide coverage for the new QA hook.
Suggested implementation:
expect(screen.getByText(collapsed)).not.toHaveClass(
'g-disclosure__collapsed-content_visible',
);
expect(screen.getByText(details)).toHaveClass('g-disclosure__content_visible');
});
test('collapsed details are visible when not expanded', () => {
const collapsed = 'Collapsed content';
const details = 'Details content';
render(
<Disclosure expanded={false}>
<Disclosure.CollapsedDetails>{collapsed}</Disclosure.CollapsedDetails>
{details}
</Disclosure>,
);
expect(screen.getByText(collapsed)).toHaveClass('g-disclosure__collapsed-content_visible');
});
test('collapsed details have default data-qa', () => {
const collapsed = 'Collapsed content';
render(
<Disclosure>
<Disclosure.CollapsedDetails>{collapsed}</Disclosure.CollapsedDetails>
</Disclosure>,
);
const component = screen.getByText(collapsed);
expect(component).toHaveAttribute('data-qa', DisclosureQa.COLLAPSED_DETAILS);
});
test('collapsed details allow overriding data-qa via qa prop', () => {
const collapsed = 'Collapsed content';
const customQa = 'custom-collapsed-details-qa';
render(
<Disclosure>
<Disclosure.CollapsedDetails qa={customQa}>{collapsed}</Disclosure.CollapsedDetails>
</Disclosure>,
);
const component = screen.getByText(collapsed);
expect(component).toHaveAttribute('data-qa', customQa);
});
- At the top of
Disclosure.test.tsx, importDisclosureQafrom the same module used by other QA-related tests in this file. For example, if other tests already use QA enums, mirror that import style:import { DisclosureQa } from '../Disclosure.qa';(or the correct relative path in your codebase).
- Ensure
Disclosure.CollapsedDetailsaccepts aqaprop and passes it through to the rendered DOM element asdata-qa, defaulting toDisclosureQa.COLLAPSED_DETAILSwhenqais not provided. This behavior should already exist or be added in theDisclosure.CollapsedDetailsimplementation to make these tests pass.
Contributor
|
Preview is ready. |
Contributor
|
🎭 Component Tests Report is ready. |
GermanVor
force-pushed
the
DisclosureCollapsedDetails
branch
from
July 16, 2026 15:53
9bd2fb8 to
2fc7fe9
Compare
Raubzeug
reviewed
Jul 24, 2026
| summary = item; | ||
| continue; | ||
| } | ||
| const isDisclosureCollapsedDetails = isDisclosureCollapsedDetailsComponent(item); |
Contributor
There was a problem hiding this comment.
Could we handle an explicit <Disclosure.Details> as a first-class child here as well? In the intended composition:
<Disclosure>
<Disclosure.CollapsedDetails>Preview</Disclosure.CollapsedDetails>
<Disclosure.Details>Full view</Disclosure.Details>
</Disclosure>the explicit Disclosure.Details currently falls through to content and is then wrapped in another DisclosureDetails below. This produces two transition containers and two role="region" elements with the same ariaControls ID; with CollapsedDetails, both content layers also receive the transition classes.
Could we preserve an explicit Disclosure.Details instance (and define the behavior when it is mixed with raw children) and add a regression test using the exact composition above?
Raubzeug
reviewed
Jul 24, 2026
| // Inside the shared body wrapper the disappearing content is taken out of the | ||
| // flow and overlaid on top of the appearing one, so their heights don't briefly | ||
| // sum up and make the layout jump. At rest, both stay in the normal flow. | ||
| &__body &__content#{$block}_exit_active, |
Contributor
There was a problem hiding this comment.
On collapse, __content is the exiting element, so this rule removes the full details from normal flow immediately. If the expanded details are taller than the collapsed preview, __body shrinks to the preview height and the following sibling moves up while the outgoing details are still painted for 100 ms, causing them to overlap.
This is the layout jump/overlap that the new API is intended to avoid. Could we instead keep the two direct children in the same CSS Grid area so the track uses the larger height while both are present, or otherwise preserve/animate the __body height during the transition?
It would also be useful to cover this with an interaction component test that collapses details taller than the preview and checks the position of the following sibling during the transition; the current jsdom tests only assert class changes.
Raubzeug
reviewed
Jul 24, 2026
| nodeRef={containerRef} | ||
| in={visible} | ||
| addEndListener={(done) => containerRef.current?.addEventListener('animationend', done)} | ||
| classNames={getCSSTransitionClassNames(b)} |
Contributor
There was a problem hiding this comment.
The earlier Sourcery thread was marked as addressed after this logic moved out of DisclosureCollapsedDetails, but the listener lifecycle is unchanged here. Every transition adds another animationend listener, and with the default keepMounted={true} the same DOM node remains mounted, so completed and cancelled callbacks stay attached and are invoked by later animation events.
Could we register a removable one-shot handler for each transition (ideally filtering event.target === containerRef.current so animated descendants cannot complete it), or use another completion mechanism that provides cleanup?
Raubzeug
reviewed
Jul 24, 2026
| export interface DisclosureComposition { | ||
| Summary: typeof DisclosureSummary; | ||
| Details: typeof DisclosureDetails; | ||
| CollapsedDetails: typeof DisclosureCollapsedDetails; |
Contributor
There was a problem hiding this comment.
Could we reconsider the public name before merging? CollapsedDetails is somewhat difficult to interpret because “details” normally refers to the panel shown in the expanded state.
Would Disclosure.Preview describe the intended role more clearly?
<Disclosure>
<Disclosure.Preview>Short view</Disclosure.Preview>
<Disclosure.Details>Full view</Disclosure.Details>
</Disclosure>If the intended use case is instead a truncated preview of the same content, that may be better represented by a separate Spoiler component or a collapsedHeight API.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Introduce a Disclosure.CollapsedDetails subcomponent that shows overlay preview content only while the disclosure is collapsed, with supporting styling, types, stories, tests, and documentation.
New Features:
Enhancements:
Documentation:
Tests: