Add highlighting for standard links and dropdown #1414
Conversation
There was a problem hiding this comment.
Thanks! @jonahtanjz Really clean implementation
Some comments, and one open question:
| @@ -132,9 +132,10 @@ | |||
| const navLis = Array.from(this.$el.querySelector('.navbar-nav').children); | |||
| // attempt an exact match first | |||
| for (const li of navLis) { | |||
| const standardLinks = [li]; | |||
There was a problem hiding this comment.
hmm what's this for? 😮
tried removing this and it worked fine still
There was a problem hiding this comment.
Oh this is for cases where the navbar does not use the li tags.
For example:
<header>
<navbar type="dark">
<a slot="brand" href="{{baseUrl}}/index.html" title="Home" class="navbar-brand">Your Logo</a>
<a href="{{baseUrl}}/contents/topic1.html" class="nav-link">Topic 1</a>
<a href="{{baseUrl}}/contents/topic2.html" class="nav-link">Topic 2</a>
<a href="{{baseUrl}}/contents/topic3a.html" class="nav-link">Topic 3a</a>
<a href="{{baseUrl}}/contents/topic3b.html" class="nav-link">Topic 3b</a>
</navbar>
</header>
In this case, without this line, the highlighting would not work properly as navLinks only looks for anchortags within li.
I'm not sure if the above navbar example is valid as each item is not within a li tag. If it is not supposed to be valid, then this line, const standardLinks = [li];, is not needed.
There was a problem hiding this comment.
thanks for the explanation; nice catch 👍
| @@ -160,16 +165,25 @@ | |||
| if (hlMode === 'sibling-or-child') { | |||
| if (this.isSibling(url, a.href) || this.isChild(url, a.href)) { | |||
| li.classList.add('current'); | |||
| if (dropdownLinks.includes(a)) { | |||
There was a problem hiding this comment.
let's move L167-170 to a inner function and use that - less repitition
There was a problem hiding this comment.
Alright thanks. I have shifted this to an inner function.
| @@ -463,3 +463,8 @@ button.copy-btn { | |||
| .navbar.navbar-dark.bg-primary .navbar-nav .current { | |||
| background: rgba(255, 255, 255, 0.05); | |||
| } | |||
|
|
|||
| .dropdown-current { | |||
There was a problem hiding this comment.
you could use vue loader's deep selectors here to encapsulate it in navbar.vue
There was a problem hiding this comment.
I have moved this block into navbar.vue. Thanks.
|
Thanks for the review @ang-zeyu. One question, I've also written additional test site but when I ran |
This (ideally) needs vue snapshot tests (which tests post-vue-render diffs), not our custom functional tests which we mostly use to detect unwanted changes on the backend (automatically, without serving them up) Vue snapshot tests were added rather recently in MarkBind/vue-strap#146 though, and only for I'm ok with proceeding without tests for now, we could add them for navbars (and etc.) in a later PR together. 🙂 |
|
Thanks @ang-zeyu for the clarification. Nope I don't intend to make anymore changes and I have opened the PR now. |
What is the purpose of this pull request?
Overview of changes:
Fixes #1390. This adds highlighting to individual links/anchortags, as well as individual dropdown items. For anchortags, if
window.location.hrefmatches itsa.href, it will be assigned thecurrentclass. For dropdowns, the parent (dropdown header) will be assigned thecurrentclass and the child (dropdown item) will be assigned thedropdown-currentclass which will highlight both the parent and child links.Anything you'd like to highlight / discuss:
Implementation and code quality
Testing instructions:
Proposed commit message: (wrap lines at 72 characters)
Extend navbar highlighting to standard links and dropdowns
Checklist: ☑️