-
Notifications
You must be signed in to change notification settings - Fork 140
Fix navigation anchor parsing #2751
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2751 +/- ##
==========================================
+ Coverage 61.36% 61.65% +0.29%
==========================================
Files 130 130
Lines 7170 7167 -3
Branches 1593 1503 -90
==========================================
+ Hits 4400 4419 +19
+ Misses 2580 2558 -22
Partials 190 190 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Use best match instead of first match
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes navigation anchor parsing issues in both site navigation and navbar components. The bug occurred when markdown lists in site navigation used loose list formatting (which wraps anchors in <p> tags) instead of tight lists, and when navbar highlighting selected the first matching link rather than the most specific one.
Key Changes:
- Updated site navigation to find anchors anywhere within list items instead of only as direct children
- Refactored navbar highlighting to select the best (most specific) match instead of the first match
- Added test coverage for the navbar highlighting logic
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/core/src/html/siteAndPageNavProcessor.ts | Changed anchor selector from direct children to descendant search to handle loose markdown lists |
| packages/vue-components/src/Navbar.vue | Refactored highlighting logic to find the best match based on path specificity instead of returning on first match |
| packages/vue-components/src/tests/Navbar.spec.js | Added test case verifying that the most specific link is highlighted over broader matches |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
What is the purpose of this pull request?
Overview of changes:
Closes #2752
Note that some other specific html that is used can cause this bug too, including nested anchors
The issue is due to expected html for the lists used in site nav to be tight markdown lists (Use https://markdowntohtml.com/),versus using loose markdown lists which is generated to different html format (nests the anchors in a
<p></p>element.Anything you'd like to highlight/discuss:
Fix for SiteNav
Fix the logic by simply recognizing it as an URL anchor if there is a inside the list point instead of it being a direct child to fix these issues.
Fix for Navbar
The previous implementation is that it takes two passes, first pass checks for an exact match in URL, while the second pass finds the first link tha that fits the criteria (default is
sibling-or-child, see navbar spec ), and terminates once it finds the first possible match.This is the issue as it does not find the best match, but rather first match. This means that a base link would always match and hence return early even when a more specific link is present.
Testing instructions:
Nil
Proposed commit message: (wrap lines at 72 characters)
Fix site-nav anchor parsing and navbar link highlighting
Checklist: ☑️
Reviewer checklist:
Indicate the SEMVER impact of the PR:
At the end of the review, please label the PR with the appropriate label:
r.Major,r.Minor,r.Patch.Breaking change release note preparation (if applicable):