Skip to content
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

Breadcrumbs: Only dedupe breacrumb items for matching node names #78077

Merged
merged 2 commits into from
Nov 16, 2023

Conversation

gillesdemey
Copy link
Member

@gillesdemey gillesdemey commented Nov 13, 2023

What is this feature?

This PR is a follow-up to #75218 but changes the logic to only de-duplicate breadcrumb items when both the base URL and the node names match.

Why do we need this feature?

The new alerting detail page (WIP) injects two additional breadcrumbs when viewing the details for a particular alert.

A namespace and the group breadcrumb are added; and we'd like both of them to navigate to the alert rules list view but with some search filters applied.

The breadcrumb would roughly look like Alerting > My Namespace > My group > My alert rule.

My Namespace would go to /alerting/list?query=namespace:"my+namespace"
My Group would go to /alerting/list?query=namespace:"my+namespace"+group="my+group"

Both breadcrumb items share the same base path, which currently triggers de-duplication logic in the breadcrumb component.

const isSamePathAsLastBreadcrumb = urlToMatch.length > 0 && lastPath === urlToMatch;
// Remember this path for the next breadcrumb
lastPath = urlToMatch;
if (!node.hideFromBreadcrumbs && !isSamePathAsLastBreadcrumb) {
crumbs.unshift({ text: node.text, href: node.url ?? '' });
}

This PR adds an additional requirement for both the current and the parent breadcrumb item to also share the same text.

Special notes for your reviewer:

This is a proposed change to the breadcrumbs de-dupe logic, feel free to discuss alternatives :)

@gillesdemey gillesdemey marked this pull request as ready for review November 14, 2023 15:07
@gillesdemey gillesdemey requested a review from a team as a code owner November 14, 2023 15:07
@gillesdemey gillesdemey requested review from tskarhed, eledobleefe, ashharrison90 and joshhunt and removed request for a team November 14, 2023 15:07
Copy link
Contributor

@ashharrison90 ashharrison90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 👍 let's get this in to unblock

@ashharrison90 ashharrison90 merged commit 2659409 into main Nov 16, 2023
19 checks passed
@ashharrison90 ashharrison90 deleted the gilles/breadcrumbs-dedupe-name branch November 16, 2023 13:46
@aangelisc aangelisc modified the milestones: 10.3.x, 10.2.3 Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants