Add footnotes component #707
Conversation
🦋 Changeset detectedLatest commit: fe9283d The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
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 |
🟢 No design token changes found |
🟢 No visual differences foundOur visual comparison tests did not find any differences in the UI. |
| className={clsx(styles.Footnotes, styles['Footnotes--variant-citations'], className)} | ||
| {...(rest as BaseProps<HTMLOListElement>)} | ||
| > | ||
| {children} |
There was a problem hiding this comment.
Do you think we need to guard against someone passing as="ol" with variant="disclaimer"? That would render an <ol> with <Text> children.
There was a problem hiding this comment.
Good question. variant isn't documented anywhere outside of source code, and I'm using it as an internal-only prop. Bit of a trade-off between adding more code to guard it vs the managing the complexity of adding them.
In this case, because its undocumented I think i prefer to not add the code, though I have now prepended _ to the start to make it clearer for TS users. Maybe this is enough of a guard? Could also go more heavy handed, add warnings, pass the as prop down and add conditional logic to prevent this configuration from being possible but we could always add that down the line too if we're seeing a lot of misuse.
| it('renders the default footnotes correctly', async () => { | ||
| const items = ['Citation 1', 'Citation 2', 'Citation 3'] | ||
|
|
||
| const {getByTestId, container} = render( | ||
| <Footnotes data-testid="footnotes"> | ||
| {items.map((citation, index) => ( | ||
| <Footnotes.Item data-testid={`item-${index}`} key={citation}> | ||
| {citation} | ||
| </Footnotes.Item> | ||
| ))} | ||
| </Footnotes>, | ||
| ) | ||
|
|
||
| const results = await axe(container) | ||
| expect(results).toHaveNoViolations() | ||
|
|
||
| const rootEl = getByTestId('footnotes') | ||
|
|
||
| expect(rootEl).toBeInTheDocument() | ||
| expect(rootEl.tagName).toEqual('OL') | ||
|
|
||
| let index = 0 | ||
| for (const item of items) { | ||
| const itemEl = getByTestId(`item-${index}`) | ||
| expect(itemEl).toHaveTextContent(item) | ||
| expect(itemEl).toBeInTheDocument() | ||
| expect(itemEl.tagName).toEqual('LI') | ||
| index++ | ||
| } | ||
| }) |
There was a problem hiding this comment.
Just a suggestion, but you could get rid of test-ids completely here. Testing Library lists them as their lowest-priority queries and as such they recommend avoiding them unless absolutely necessary
it('renders the default footnotes correctly', async () => {
const items = ['Citation 1', 'Citation 2', 'Citation 3']
const {getByRole, container} = render(
<Footnotes>
{items.map((citation, index) => (
<Footnotes.Item data-testid={`item-${index}`} key={citation}>
{citation}
</Footnotes.Item>
))}
</Footnotes>,
)
const results = await axe(container)
expect(results).toHaveNoViolations()
const listEl = getByRole('list')
expect(listEl).toBeInTheDocument()
expect(listEl.tagName).toEqual('OL')
for (const item of items) {
expect(getByRole('listitem', {name: item})).toBeInTheDocument()
}
})There was a problem hiding this comment.
Thanks.
expect(getByRole('listitem', {name: item})).toBeInTheDocument()
☝️ doesn't work because of the way the content is nested inside another tag of the listitem. Relevant WCAG guidelines here. I've instead used the textContent property on the listitem to infer the correct value. IMO it's a bit more convoluted now, but happy to keep it like this.
If you're wondering why the assertion against tagName is still there, it's because the listitem role can be manually assigned to a different tag, so this makes sure that doesn't happen/
|
From a design perspective, the footnotes highlight doesn't really work. In dark mode, the contrast between the text and background is insufficient (4.22). Additionally, the yellow in dark mode clashes with the rest of the page. Finally, the highlight should only wrap the text, including the counter, instead of going full-width. To unblock this PR. I suggest we remove the highlight from this initial version of the component while we figure out a better visual treatment. |
@danielguillan the target highlight has been removed |
Summary
Part of https://github.com/github/primer/issues/3673
Adds a new Footnotes component to the library.
Displays two types of footnotes content:
citationanddisclaimer.🔗 Documentation preview
🔗 Real-world example
🔗 Citation examples in Storybook
🔗 Paragraph (disclaimer) examples in Storybook
List of notable changes:
What should reviewers focus on?
Steps to test:
Use the following links:
Supporting resources (related issues, external links, etc):
Contributor checklist:
Reviewer checklist:
Screenshots: