fix(core/breadcrumb): add missing aria-label to next items dropdown button of breadcrumb - #2683
fix(core/breadcrumb): add missing aria-label to next items dropdown button of breadcrumb #2683lakshmi-priya-b wants to merge 3 commits into
Conversation
✅ Deploy Preview for ix-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
📝 WalkthroughWalkthroughThe breadcrumb next-items dropdown derives the last breadcrumb label from its ChangesBreadcrumb accessibility
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/components/breadcrumb/breadcrumb.tsx`:
- Line 211: Add a changeset for the consumer-visible accessibility update in the
core package, describing the breadcrumb aria-label behavior change and assigning
the appropriate release impact. If this requires a major release, include
migration guidance; otherwise, keep the changeset scoped to the relevant
package.
- Line 211: Update the last-item breadcrumb rendering to use the same trimmed
text fallback as BreadcrumbItem.render(), preferring label and then
hostElement.textContent?.trim() rather than innerText. Compute this fallback
once and reuse it for both the label at the surrounding render logic and the
aria-label, and add a regression case covering slotted text when the item is
invisible.
In `@packages/core/src/components/breadcrumb/test/breadcrumb.ct.ts`:
- Around line 20-30: Update the accessibility regression test around the
breadcrumb mount and axe analysis to provide nextItems so
ix-dropdown-button.next-button renders, then use a Playwright locator to assert
its inner button has the accessible name “Show Item 3” via getByRole before or
alongside the axe assertion. Keep the existing zero-violations check and ensure
the test covers the changed button behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cd3f2d81-9a18-403e-ae38-773308b2c51e
📒 Files selected for processing (2)
packages/core/src/components/breadcrumb/breadcrumb.tsxpackages/core/src/components/breadcrumb/test/breadcrumb.ct.ts
| variant="tertiary" | ||
| enableTopLayer={this.enableTopLayer} | ||
| aria-current="page" | ||
| aria-label={`Show ${labelLastItem.label ?? labelLastItem.innerText}`} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add the required changeset.
This change modifies consumer-visible accessibility behavior in packages/core. Add a changeset with the relevant package scope, or explicitly justify why the change is internal-only. Include migration guidance if this is a major release change.
As per path instructions, changesets are required for accessibility and other consumer-relevant behavior changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/components/breadcrumb/breadcrumb.tsx` at line 211, Add a
changeset for the consumer-visible accessibility update in the core package,
describing the breadcrumb aria-label behavior change and assigning the
appropriate release impact. If this requires a major release, include migration
guidance; otherwise, keep the changeset scoped to the relevant package.
Source: Path instructions
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reuse BreadcrumbItem's text fallback.
BreadcrumbItem.render() in packages/core/src/components/breadcrumb-item/breadcrumb-item.tsx uses label ?? hostElement.textContent?.trim(). This code uses innerText instead. When the last item uses slotted text and becomes invisible, innerText can omit that text. The dropdown can then expose aria-label="Show " or a different name.
Compute one trimmed fallback and reuse it for the label at Line 206 and this aria-label. Add a regression case with slotted text.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/components/breadcrumb/breadcrumb.tsx` at line 211, Update
the last-item breadcrumb rendering to use the same trimmed text fallback as
BreadcrumbItem.render(), preferring label and then
hostElement.textContent?.trim() rather than innerText. Compute this fallback
once and reuse it for both the label at the surrounding render logic and the
aria-label, and add a regression case covering slotted text when the item is
invisible.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/components/breadcrumb/breadcrumb.tsx`:
- Around line 208-213: Add a changeset for the core package to document the
consumer-visible accessibility change introduced in breadcrumb.tsx by the new
aria-label on the next button. Update the Breadcrumb component change and
include the accessibility behavior and release impact in the changeset, or if
you believe it is internal-only, explicitly justify that in the same changeset;
use the ix-breadcrumb/Breadcrumb symbols to keep the scope tied to this
consumer-facing API change.
In `@packages/core/src/components/breadcrumb/test/breadcrumb.ct.ts`:
- Around line 20-39: Add a breadcrumb item without a label and provide its
visible text through slotted content in the accessibility fixture, so the
textContent fallback in the breadcrumb component executes. Keep the existing
Playwright locator assertion and verify the next button still has the accessible
name “Show Item 3 next items”.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6639d1ae-8d64-4c6a-9086-af37cc38f37b
📒 Files selected for processing (2)
packages/core/src/components/breadcrumb/breadcrumb.tsxpackages/core/src/components/breadcrumb/test/breadcrumb.ct.ts
| label={labelLastItemText} | ||
| class="next-button" | ||
| variant="tertiary" | ||
| enableTopLayer={this.enableTopLayer} | ||
| aria-current="page" | ||
| aria-label={`Show ${labelLastItemText} next items`} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add the required changeset before merge.
The new aria-label changes the consumer-visible accessibility contract of ix-breadcrumb. No changeset is included in the provided PR files. Add a changeset scoped to the core package with the accessibility behavior and release impact, or explicitly justify why this change is internal-only.
As per path instructions, changesets are required for accessibility and other consumer-relevant behavior changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/components/breadcrumb/breadcrumb.tsx` around lines 208 -
213, Add a changeset for the core package to document the consumer-visible
accessibility change introduced in breadcrumb.tsx by the new aria-label on the
next button. Update the Breadcrumb component change and include the
accessibility behavior and release impact in the changeset, or if you believe it
is internal-only, explicitly justify that in the same changeset; use the
ix-breadcrumb/Breadcrumb symbols to keep the scope tied to this consumer-facing
API change.
Source: Path instructions
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/components/breadcrumb/test/breadcrumb.ct.ts`:
- Around line 20-25: Update the accessibility fixture in
regressionTest('accessibility') to include a nextItems entry while preserving
the unlabeled slotted Item 3 text fallback, then assert the rendered inner
button using the Playwright locator getByRole('button', { name: 'Show Item 3
next items' }).
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6b8b3f1b-b92c-4b55-816b-0735aeb22e8a
📒 Files selected for processing (1)
packages/core/src/components/breadcrumb/test/breadcrumb.ct.ts
1 participant
💡 What is the current behavior?
The next items dropdown button had no
aria-label, making it unannounced and non-navigable for screen reader users.JIRA: IX-4347
🆕 What is the new behavior?
The next items dropdown button had now
aria-label, making it announced and navigable for screen reader users.🏁 Checklist
A pull request can only be merged if all of these conditions are met (where applicable):
pnpm test)pnpm lint)pnpm build, changes pushed)👨💻 Help & support
Summary by CodeRabbit