Apply correct markup to selected breadcrumb items #833
Conversation
🦋 Changeset detectedLatest commit: 3401070 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. |
| const breadcrumbLinkEls = getAllByRole('link') | ||
| expect(breadcrumbLinkEls).toHaveLength(2) | ||
|
|
||
| const item1 = breadcrumbLinkEls[0] | ||
| expect(item1.textContent).toBe('Resources') | ||
| expect(item1.getAttribute('href')).toBe('/') | ||
|
|
||
| const item2 = breadcrumbLinkEls[1] | ||
| expect(item2.textContent).toBe('GitHub Copilot') | ||
| expect(item2.getAttribute('href')).toBe('/copilot') |
There was a problem hiding this comment.
Purely a preference thing, but this could be simplified slightly to:
| const breadcrumbLinkEls = getAllByRole('link') | |
| expect(breadcrumbLinkEls).toHaveLength(2) | |
| const item1 = breadcrumbLinkEls[0] | |
| expect(item1.textContent).toBe('Resources') | |
| expect(item1.getAttribute('href')).toBe('/') | |
| const item2 = breadcrumbLinkEls[1] | |
| expect(item2.textContent).toBe('GitHub Copilot') | |
| expect(item2.getAttribute('href')).toBe('/copilot') | |
| expect(getAllByRole('link')).toHaveLength(2) | |
| expect(getByRole('link', {name: 'Resources'})).toHaveAttribute('href', '/') | |
| expect(getByRole('link', {name: 'GitHub Copilot'})).toHaveAttribute('href', '/copilot') |
Appreciate the current approach is more in line with the rest of the file, so feel free to disregard this 🙂
| size="100" | ||
| variant="muted" | ||
| className={clsx(styles.Breadcrumbs__link, styles['Breadcrumbs__link--selected'], className)} | ||
| aria-current={ariaCurrent ? ariaCurrent : 'page'} |
There was a problem hiding this comment.
Nit, but you could simplify this to
| aria-current={ariaCurrent ? ariaCurrent : 'page'} | |
| aria-current="page" |
and remove 'aria-current' from the props destructure above.
If the user passes their own aria-current to Breadcrumbs.Item then that will override this on the line below as part of {...rest}
Summary
Fixes #819
Changes the markup applied to selected breadcrumb items to be a non-interactive element. This removes tab focus from these items, making them inaccessible via keyboard navigation.
🔗 Preview
List of notable changes:
Steps to test:
Contributor checklist:
update snapshotslabel to the PR)Reviewer checklist:
Screenshots:
Before:
Screen.Recording.2024-11-14.at.9.41.02.AM.mov
After:
Screen.Recording.2024-11-26.at.14.10.41.mov