refactor breadcrumb to support duplicate labels#2479
refactor breadcrumb to support duplicate labels#2479SaiYugandhar03 wants to merge 4 commits intosiemens:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 59e9f8e 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the breadcrumb component to include an index in the itemClick event payload, enabling unique identification of items even when labels are duplicated. The changes span the core component, its tests, and the Angular, React, and Vue wrappers. Feedback includes a missing @element() decorator for hostElement in the breadcrumb item and a suggestion to use the map index for better efficiency when rendering dropdown items.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the breadcrumb component to support identifying items by index, which is particularly useful when duplicate labels exist. The itemClick event payload has been changed from a string to an object containing both the label and the index. This change is reflected across Angular, React, and Vue wrappers. Feedback highlights that this is a breaking change for consumers and suggests using textContent instead of innerText to ensure labels are correctly retrieved even when elements are hidden or styled.
| * Crumb item clicked event | ||
| */ | ||
| @Event() itemClick!: EventEmitter<string>; | ||
| @Event() itemClick!: EventEmitter<{ label: string; index: number }>; |
There was a problem hiding this comment.
Changing the itemClick event payload from a string to an object { label: string; index: number } is a breaking change for existing consumers of this component. While this is necessary to support duplicate labels, it will require updates in all applications using this event. Please ensure this is documented as a breaking change and consider if a migration path or a new event name (e.g., itemSelect) should be used to maintain backward compatibility.
|
danielleroux
left a comment
danielleroux
left a comment
There was a problem hiding this comment.
Breaking changes documentation in the BREAKING CHANGES v5.md is missing. Please add.
There was a problem hiding this comment.
Add a breaking changes section. Look into the other major changesets
4 participants
💡 What is the current behavior?
The ix-breadcrumb component cannot support duplicate item labels because it uses labels as unique identifiers.
Jira ticket: IX-4136
🆕 What is the new behavior?
The breadcrumb component works when multiple breadcrumb items have the same label.
🏁 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