Active sidebar link highlight #272

ghost · 2018-04-27T00:12:34Z

This is still a work in progress, but I'd really like your feedback on the approach so far.

The main idea here is to gather up all of the anchors, then change the hash when the user scrolls beyond one of the anchor headers. I am filtering the anchors so that we only set the hash to links in the sidebar.

Try it in Chrome/Safari, it should work really nicely! Firefox seems to have different behavior when the hash is changed, though. It jumps to the anchors as soon as they're updated. The result is very jerky scrolling, and scrolling up will immediately skip to the top of a section. Any advice for a better way to do this?

ulivz

Just a quick glance, will check detail later. maybe we can implement it without using document.querySelectorAll.

ulivz · 2018-04-27T00:51:59Z

lib/default-theme/Layout.vue

+}
+
+function gatherHeaderAnchors () {
+  const sidebarLinks = Array.from(document.querySelectorAll('.sidebar-group-items a.sidebar-link'))


Use array spread instead of Array.from, since buble doesn't polyfill for that in IE11.

I tried spread initially, but it seems to have some problems with NodeLists (https://gitlab.com/Rich-Harris/buble/issues/81). I would really prefer to use it here instead though. Do you have a preference for something else, assuming spread wouldn't work?

ulivz · 2018-04-27T00:52:50Z

lib/default-theme/Layout.vue

+  const sidebarLinks = Array.from(document.querySelectorAll('.sidebar-group-items a.sidebar-link'))
+
+  const anchors = Array.from(document.querySelectorAll('a.header-anchor'))
+    .filter(x => sidebarLinks.map(x => x.hash).includes(x.hash))


Same here, Array.prototype.includes. you can use .some instead

ghost · 2018-04-27T02:24:19Z

Updated to use router.replace to change the hash. The hash changes while scrolling seem to work well in Firefox when the scroll behavior is prevented (I have disabled it temporarily, just for testing). I wonder if there's a way to disable it only when we're making the router.replace call to change the hash?

ycmjason

not so sure about the querySelectorAll, left some comment, otherwise LGTM

ycmjason · 2018-04-27T08:40:18Z

lib/default-theme/Layout.vue

@@ -29,6 +29,7 @@ import Page from './Page.vue'
 import Sidebar from './Sidebar.vue'
 import { pathToComponentName } from '@app/util'
 import { resolveSidebarItems } from './util'
+import throttle from 'lodash/throttle'


Shouldn't this be lodash.throttle?

yes, it should be lodash.throttle.

ulivz · 2018-04-27T10:33:46Z

lib/default-theme/Layout.vue

@@ -111,6 +112,8 @@ export default {
    this.$watch('$page', updateMeta)
    updateMeta()

+    window.addEventListener('scroll', this.onScroll)


You asked a contributor to access window at created hook? Are you kidding me?

Emmm... In addtion, what is

after mounted() but before created()

@ycmjason Pay attention to your wording. If I don't read your comment, then you will be conveying a very low-level error in a vue official project. Is this reasonable?

BTW, please take a good look at Vue before you speak anything, or I'll think you're just making trouble.

@ulivz please pay attention to yours as well. You are all contributors. You all can make mistakes and can have different opinions. Point out problems with neutral language, don't bring your ego into this.

Thank you. come back and see, I really shouldn't bring my feelings into it, and should be more neutral. I'm sorry.
For above problem, the scroll event should be handled in mount or beforeMount hook, since we are building a SSR application which has some browser api access restrictions.

Opps, I must have misread previously.

Tried to removed this whole thread but turns out only be able to remove my own comment. I was not asking a contributor to access window at created(). Just genuinely curious why he put it in mounted() as I obviously just got messed up in my head about the order of created() and mounted() somehow. Thanks @ulivz for clarifying this.

I will keep these comments, although I want to delete them 😂
Just want it alert me to no longer making the same mistake in the future.

My original intention is that I do not want to import any mistake (I really love this project), but my expression is obviously too excited, sorry again.

Thanks for your contributions for VuePress.

I feel really sorry to make you feel that I am offending you all the time. In case you feel the same again, please do tell me and perhaps suggest a way that I should have put it.

🎉

Work hard together to make this project better. Come on! 🎉

ulivz · 2018-04-27T10:33:46Z

lib/default-theme/Layout.vue

@@ -111,6 +112,8 @@ export default {
    this.$watch('$page', updateMeta)
    updateMeta()

+    window.addEventListener('scroll', this.onScroll)


You asked a contributor to access window at created hook? Are you kidding me?

ulivz · 2018-04-27T14:53:17Z

Thanks for the great work! I test it with different browsers in my local area. it looks very good.

add active sidebar link highlight

bc3336b

ghost changed the title ~~Active sidebar link highlight~~ Active sidebar link highlight #245 Apr 27, 2018

ghost changed the title ~~Active sidebar link highlight #245~~ Active sidebar link highlight Apr 27, 2018

ghost mentioned this pull request Apr 27, 2018

Feature Request: Highlight current region in sidebar #245

Closed

ulivz added the semver: minor label Apr 27, 2018

ulivz reviewed Apr 27, 2018

View reviewed changes

use router to update hash

30800ac

ycmjason reviewed Apr 27, 2018

View reviewed changes

ulivz reviewed Apr 27, 2018

View reviewed changes

ryanrivest and others added 3 commits April 27, 2018 10:25

update throttle import

ffdd8b7

chore: remove comments

7ca5f5c

chore: simplify code

c67c64a

ulivz added 2 commits April 27, 2018 22:54

chore: change throttle duration from 200 to 300

58f4b02

chore: tweaks

1c4c376

ulivz merged commit 6b6d268 into vuejs:master Apr 27, 2018

ghost deleted the sidebar-highlight branch April 27, 2018 17:35

meteorlxy mentioned this pull request Apr 29, 2018

Router scrollBehavior to.hash fail after #272 #298

Closed

ulivz mentioned this pull request May 11, 2018

[Pre Release] 0.9.0 🚀 #419

Closed

ulivz added a commit that referenced this pull request May 17, 2018

fix: highlight active link (#272) doesn't work with non-EN hash.

a51a31b

achendrick mentioned this pull request May 22, 2018

Add ability to disable setActiveHash (6b6d268) #482

Closed

Qdigital mentioned this pull request Nov 5, 2022

[Snyk] Security upgrade stylus-loader from 3.0.2 to 4.0.0 qteam1/vuepress#38

Open

faizulho mentioned this pull request Nov 5, 2022

[Snyk] Security upgrade stylus-loader from 3.0.2 to 4.0.0 sgmelayu/vuepress#77

Open

snyk-bot mentioned this pull request Dec 26, 2022

[Snyk] Security upgrade stylus-loader from 3.0.2 to 4.0.0 qteam1/vuepress#41

Open

faizulho mentioned this pull request Dec 26, 2022

[Snyk] Security upgrade stylus-loader from 3.0.2 to 4.0.0 sgmelayu/vuepress#80

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Active sidebar link highlight #272

Active sidebar link highlight #272

ghost commented Apr 27, 2018

ulivz left a comment

ulivz Apr 27, 2018

ghost Apr 27, 2018

ulivz Apr 27, 2018 •

edited

Loading

ghost commented Apr 27, 2018

ycmjason left a comment

ycmjason Apr 27, 2018

ulivz Apr 27, 2018

ulivz Apr 27, 2018 •

edited

Loading

meteorlxy Apr 27, 2018

ulivz Apr 27, 2018 •

edited

Loading

yyx990803 Apr 27, 2018

ulivz Apr 27, 2018

ycmjason Apr 27, 2018

ulivz Apr 27, 2018

ycmjason Apr 27, 2018

ulivz Apr 27, 2018

ulivz Apr 27, 2018 •

edited

Loading

ulivz commented Apr 27, 2018

Active sidebar link highlight #272

Active sidebar link highlight #272

Conversation

ghost commented Apr 27, 2018

ulivz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ulivz Apr 27, 2018 • edited Loading

Choose a reason for hiding this comment

ghost commented Apr 27, 2018

ycmjason left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ulivz Apr 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ulivz Apr 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ulivz Apr 27, 2018 • edited Loading

Choose a reason for hiding this comment

ulivz commented Apr 27, 2018

ulivz Apr 27, 2018 •

edited

Loading

ulivz Apr 27, 2018 •

edited

Loading

ulivz Apr 27, 2018 •

edited

Loading

ulivz Apr 27, 2018 •

edited

Loading