Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 677
feat(Details): Add summary subcomponent#5015
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
f334af7cbae243c223dbbdf23a603f449b5bac525a1eac3fb3421e392d3b1b8c719ca303a43f670379c616f1783560813723ebc11773392f28cfdc3aed3b3275e505e767a70a7885485a8b604ee9831996ad6f48b9dbc27b288bc61f33300a86ae4d5c99c4588c284ee6ce3b3File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@primer/react": minor | ||
| --- | ||
| feat(Details): Add summary subcomponent |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -12,7 +12,7 @@ export const Default: StoryFn<typeof Details> = () => { | ||
| const {getDetailsProps} = useDetails({closeOnOutsideClick: true}) | ||
| return ( | ||
| <Details {...getDetailsProps()}> | ||
| <Button as="summary">See Details</Button> | ||
| <Details.Summary as={Button}>See Details</Details.Summary> | ||
Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Couple noob questions here:
MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. | ||
| This is some content | ||
| </Details> | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| import {render} from '@testing-library/react' | ||
| import {render, screen} from '@testing-library/react' | ||
| import userEvent from '@testing-library/user-event' | ||
| import React from 'react' | ||
| import React, {act} from 'react' | ||
| import {Details, useDetails, Box, Button} from '../..' | ||
| import type {ButtonProps} from '../../Button' | ||
| import {behavesAsComponent, checkExports} from '../../utils/testing' | ||
| @@ -14,29 +14,36 @@ describe('Details', () => { | ||
| }) | ||
| it('should have no axe violations', async () => { | ||
| const {container} = render(<Details />) | ||
| const results = await axe.run(container) | ||
| const {container} = render( | ||
| <Details> | ||
| <Details.Summary>Summary</Details.Summary>Content | ||
| </Details>, | ||
| ) | ||
| let results | ||
| await act(async () => { | ||
| results = await axe.run(container) | ||
| }) | ||
| expect(results).toHaveNoViolations() | ||
| }) | ||
| it('Toggles when you click outside', () => { | ||
| it('Toggles when you click outside', async () => { | ||
| const Component = () => { | ||
| const {getDetailsProps} = useDetails({closeOnOutsideClick: true}) | ||
| return ( | ||
| <Details data-testid="details" {...getDetailsProps()}> | ||
| <summary>hi</summary> | ||
| <Details.Summary>hi</Details.Summary> | ||
| </Details> | ||
| ) | ||
| } | ||
| const {getByTestId} = render(<Component />) | ||
| const {findByTestId} = render(<Component />) | ||
| document.body.click() | ||
| expect(getByTestId('details')).not.toHaveAttribute('open') | ||
| expect(await findByTestId('details')).not.toHaveAttribute('open') | ||
| }) | ||
| it('Accurately passes down open state', () => { | ||
| it('Accurately passes down open state', async () => { | ||
| const Component = () => { | ||
| const {getDetailsProps, open} = useDetails({closeOnOutsideClick: true}) | ||
| return ( | ||
| @@ -46,12 +53,12 @@ describe('Details', () => { | ||
| ) | ||
| } | ||
| const {getByTestId} = render(<Component />) | ||
| const {findByTestId} = render(<Component />) | ||
| document.body.click() | ||
| expect(getByTestId('summary')).toHaveTextContent('Closed') | ||
| expect(getByTestId('details')).not.toHaveAttribute('open') | ||
| expect(await findByTestId('summary')).toHaveTextContent('Closed') | ||
| expect(await findByTestId('details')).not.toHaveAttribute('open') | ||
| }) | ||
| it('Can manipulate state with setOpen', async () => { | ||
| @@ -95,4 +102,53 @@ describe('Details', () => { | ||
| expect(getByTestId('summary')).toHaveTextContent('Open') | ||
| }) | ||
| it('Adds default summary if no summary supplied', async () => { | ||
| const {getByText} = render(<Details data-testid="details">content</Details>) | ||
| expect(getByText('See Details')).toBeInTheDocument() | ||
| expect(getByText('See Details').tagName).toBe('SUMMARY') | ||
| }) | ||
| it('Does not add default summary if summary supplied', async () => { | ||
| const {findByTestId, findByText} = render( | ||
| <Details data-testid="details"> | ||
| <Details.Summary data-testid="summary">summary</Details.Summary> | ||
| content | ||
| </Details>, | ||
Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (non blocker) Should this also be a dev warning? example of warning MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. superseeded by bac525a | ||
| ) | ||
| await expect(findByText('See Details')).rejects.toThrow() | ||
| expect(await findByTestId('summary')).toBeInTheDocument() | ||
| expect((await findByTestId('summary')).tagName).toBe('SUMMARY') | ||
| }) | ||
| it('Does not add default summary if supplied as different element', async () => { | ||
| const {findByTestId, findByText} = render( | ||
| <Details data-testid="details"> | ||
| <Box as="summary" data-testid="summary"> | ||
| custom summary | ||
| </Box> | ||
| content | ||
| </Details>, | ||
| ) | ||
| await expect(findByText('See Details')).rejects.toThrow() | ||
| expect(await findByTestId('summary')).toBeInTheDocument() | ||
| expect((await findByTestId('summary')).tagName).toBe('SUMMARY') | ||
| }) | ||
| describe('Details.Summary', () => { | ||
| behavesAsComponent({Component: Details.Summary, options: {skipSx: true}}) | ||
| it('should support a custom `className` on the container element', () => { | ||
| render(<Details.Summary className="custom-class">test summary</Details.Summary>) | ||
| expect(screen.getByText('test summary')).toHaveClass('custom-class') | ||
| }) | ||
| it('should pass extra props onto the container element', () => { | ||
| render(<Details.Summary data-testid="test">test summary</Details.Summary>) | ||
| expect(screen.getByText('test summary')).toHaveAttribute('data-testid', 'test') | ||
| }) | ||
| }) | ||
| }) | ||



Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(blocking)
summaryprop 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.Summaryand 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
Detailscomponent is used but neitherDetails.Summarynor a nativesummaryis present within thechildren).