-
Notifications
You must be signed in to change notification settings - Fork 563
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
feat(Details): Add summary subcomponent #5015
Changes from all commits
f334af7
cbae243
c223dbb
df23a60
3f449b5
bac525a
1eac3fb
3421e39
2d3b1b8
c719ca3
03a43f6
70379c6
16f1783
5608137
23ebc11
773392f
28cfdc3
aed3b32
75e505e
767a70a
7885485
a8b604e
e983199
6ad6f48
b9dbc27
b288bc6
1f33300
a86ae4d
5c99c45
88c284e
e6ce3b3
File filter
Filter by extension
Conversations
Jump to
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> | ||
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:
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>, | ||
) | ||
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 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') | ||
}) | ||
}) | ||
}) |
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)
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.
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.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 neitherDetails.Summary
nor a nativesummary
is present within thechildren
).