Uh oh!
There was an error while loading. Please reload this page.
feat(Details): Add summary subcomponent - #5015
Conversation
🦋 Changeset detectedLatest commit: e6ce3b3 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 |
size-limit report 📦
|
| <Details summary="test summary" data-testid="details"> | ||
| <Details.Summary>custom summary</Details.Summary> | ||
| content | ||
| </Details>, |
There was a problem hiding this comment.
(non blocker) Should this also be a dev warning? example of warning
| ) | ||
| expect(queryByText('See Details')).toBeNull() | ||
| expect(getByTestId('summary')).toBeInTheDocument() |
There was a problem hiding this comment.
(just a note) TIL you can't getByRole('summary'), it's inferred as a button instead!
| } | ||
| ] | ||
| } | ||
| ] |
There was a problem hiding this comment.
(blocking) summary prop is missing from docs.
Curious, should we prefer adding just 1 approach instead of both <Details summary> and <Details.Summary>? To me, <Details.Summary> feels like the closest replacement for <summary>, which feels complete.
Would like to hear your thoughts on the 2 approaches though!
There was a problem hiding this comment.
Thought about this and I agree with you. I think originally I was going for the "maximize ease-of-use" approach but in hindsight this might be confusing and adds extra complexity with having to support the intersection of both strategies (Re: your comment here).
Going to refactor to just introduce Details.Summary and render a default one if one isn't present, that feels right-er to me as well.
Wondering, do you think that would warrant a warning as well? (The use case where a Details component is used but neither Details.Summary nor a native summary is present within the children).
| return ( | ||
| <Details {...getDetailsProps()}> | ||
| <Button as="summary">See Details</Button> | ||
| <Details.Summary as={Button}>See Details</Details.Summary> |
There was a problem hiding this comment.
Couple noob questions here:
- Assuming this renders a
<button>, is that still accessible with<details>? - Just checking, does this also support ButtonProps like
size=smalland variant=primary`?
There was a problem hiding this comment.
- Note sure what accessible with details means 😅, here's how/what it renders:
is there some specific behavior you want me to test for here?
- Had to fiddle with the types a little but yeah those work:
…/3518-prcdetails-component-does-not-provide-appropriate-structure-by-default
…/3518-prcdetails-component-does-not-provide-appropriate-structure-by-default
…/3518-prcdetails-component-does-not-provide-appropriate-structure-by-default
…does-not-provide-appropriate-structure-by-default
👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/349552 |
Closeshttps://github.com/github/primer/issues/3518
Adds
Details.Summarysubcomponent that can be used withinDetailsfor the summary and addssummaryprop toDetailscomponent which will render the supplied node within the newDetails.Summarysubcomponent. Also adds a default "See Details" summary value to be rendered to improve accessibility in case authors fail to supply one.Changelog
New
Details.SummarysubcomponentDetails.SummarysubcomponentChanged
Details.docs.jsonto include data for newDetails.Summarysubcomponent and its propsDetails.Summarysubcomponent inDetailsstoryDetailscomponent to render a defaultDetails.Summaryif one hasn't been supplied by authorDetailscomponent export to include newDetails.SummarysubcomponentDetailstests to use newDetails.SummarysubcomponentRollout strategy
Testing & Reviewing
DetailsstoryMerge checklist