Conversation
Reviewer's GuideAdds layout-related render props to Disclosure.Summary and wires them through the default summary implementation and styles, exposing DefaultDisclosureSummary as a static member on DisclosureSummary. 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}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
justifyContentvalues in types/implementation ('initial' | 'space-between'andjustify-content_space-betweenclass) don’t match the README docs ("start" | "space-between"), so please align the API, styles, and documentation on a single set of allowed values. - README now describes
widthandjustifyContentasDisclosure.Summaryprops, but in this diff they are only added toDefaultDisclosureSummaryrender props; if these are intended as top-levelSummaryprops, please wire them through the main component as well. - Consider explicitly handling the default cases for
widthandjustifyContentin styles (e.g., modifiers or clear defaults forauto/start) so that the behavior is predictable and consistent when these props are omitted.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `justifyContent` values in types/implementation (`'initial' | 'space-between'` and `justify-content_space-between` class) don’t match the README docs (`"start" | "space-between"`), so please align the API, styles, and documentation on a single set of allowed values.
- README now describes `width` and `justifyContent` as `Disclosure.Summary` props, but in this diff they are only added to `DefaultDisclosureSummary` render props; if these are intended as top-level `Summary` props, please wire them through the main component as well.
- Consider explicitly handling the default cases for `width` and `justifyContent` in styles (e.g., modifiers or clear defaults for `auto`/`start`) so that the behavior is predictable and consistent when these props are omitted.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Preview is ready. |
|
🎭 Component Tests Report is ready. |
Raubzeug
left a comment
Raubzeug
left a comment
There was a problem hiding this comment.
One API issue to address:
| disabled?: boolean; | ||
| className?: string; | ||
| width?: DisclosureSummaryWidth; | ||
| justifyContent?: DisclosureSummaryJustifyContent; |
There was a problem hiding this comment.
width and justifyContent are documented as Disclosure.Summary props, but they are declared on DisclosureSummaryRenderFunctionProps. DisclosureSummary does not include either value in the object passed to the render function, while DisclosureSummaryProps does not accept them at all. As a result, the API shown in the README cannot type-check. Could we move these options to DisclosureSummaryProps and pass them to the default summary when it is created?
There was a problem hiding this comment.
Thanks — the new props are now accepted by DisclosureSummaryProps, but the render-function contract is still expanded and all layout/content values are included in props (lines 74–86). This breaks the documented {(props) => <Button {...props}>…} path: a size="xl" Disclosure now changes the custom Button to xl and adds width_auto, while summary is forwarded to the DOM and React warns about justifyContent/arrowPosition. I reproduced this against the current head. Could we keep render props interaction-only and introduce separate DefaultDisclosureSummaryProps (or pass these values only when building defaultSummary)?
| } | ||
|
|
||
| export type DisclosureSummaryWidth = 'auto' | 'max'; | ||
| export type DisclosureSummaryJustifyContent = 'initial' | 'space-between'; |
There was a problem hiding this comment.
The implementation accepts "initial" | "space-between", while both READMEs document "start" | "space-between". This makes the documented justifyContent="start" value invalid in TypeScript. Could we use the same values throughout? I would prefer "start" | "space-between" because initial is a CSS-wide keyword rather than a semantic layout option.
| ); | ||
| }); | ||
|
|
||
| DisclosureSummary.Default = DefaultDisclosureSummary; |
There was a problem hiding this comment.
Non-blocking suggestion: if width and justifyContent become DisclosureSummaryProps, would it make sense to make children optional and render defaultSummary when it is omitted? That would support <Disclosure.Summary width="max" justifyContent="space-between" /> without forcing consumers to recreate Disclosure.Summary.Default and spread the interaction props manually. The existing render-function behavior could remain unchanged whenever children is provided.
There was a problem hiding this comment.
had to add more props for <Disclosure.Summary width="max" justifyContent="space-between" />
| width: 100%; | ||
| } | ||
| &__trigger_justify-content_space-between { | ||
| justify-content: space-between; |
There was a problem hiding this comment.
Could we add regression coverage for these new layout modifiers? At minimum, a component test should verify that width_max and justify-content_space-between are applied and that omitting the props preserves the current classes. A Storybook or visual case with an end-positioned arrow and a long summary would also verify that the full-width layout behaves as intended.
| > | ||
| <ArrowToggle | ||
| size={ComponentSizeToIconSizeMap[size]} | ||
| size={ComponentSizeToIconSizeMap[mergedSize]} |
There was a problem hiding this comment.
size currently overrides only the ArrowToggle icon through mergedSize. The trigger typography is still selected by the parent .g-disclosure_size_* .g-disclosure__trigger rules, so <Disclosure size="m"><Disclosure.Summary size="xl" /></Disclosure> renders an xl icon with m text. This contradicts the documented “overrides Disclosure.size” contract. Could we either remove this extra override from the scope, or add a trigger-level size modifier so the entire summary changes size?
| children: renderFunction, | ||
| qa, | ||
| justifyContent = 'start', | ||
| width = 'auto', |
There was a problem hiding this comment.
Defaulting these values here adds _width_auto and _justify-content_start classes (and explicit CSS declarations) to every existing Disclosure, including the implicit summary path. That changes the DOM/CSS contract and may override consumer styles, while the browser’s natural flex defaults already provide the same auto/start behavior. Could we keep both values undefined when omitted and emit modifier classes only for explicit props? The omitted-props test should then assert that these new classes are absent.
2 participants
WIP
Summary by Sourcery
Add layout customization options to Disclosure.Summary and expose the default summary component for reuse.
New Features:
Enhancements: