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

Active sidebar link highlight #272

Merged
merged 7 commits into from Apr 27, 2018
Merged

Active sidebar link highlight #272

merged 7 commits into from Apr 27, 2018

Conversation

ghost
Copy link

@ghost ghost commented Apr 27, 2018

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?

@ghost ghost changed the title Active sidebar link highlight Active sidebar link highlight #245 Apr 27, 2018
@ghost ghost changed the title Active sidebar link highlight #245 Active sidebar link highlight Apr 27, 2018
Copy link
Member

@ulivz ulivz left a comment

Choose a reason for hiding this comment

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

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

}

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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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?

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))
Copy link
Member

@ulivz ulivz Apr 27, 2018

Choose a reason for hiding this comment

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

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

@ghost
Copy link
Author

ghost commented Apr 27, 2018

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?

Copy link
Contributor

@ycmjason ycmjason left a comment

Choose a reason for hiding this comment

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

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

@@ -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'
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be lodash.throttle?

Copy link
Member

Choose a reason for hiding this comment

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

yes, it should be lodash.throttle.

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

window.addEventListener('scroll', this.onScroll)
Copy link
Member

@ulivz ulivz Apr 27, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Emmm... In addtion, what is

after mounted() but before created()

Copy link
Member

@ulivz ulivz Apr 27, 2018

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

🎉

Copy link
Member

Choose a reason for hiding this comment

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

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

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

window.addEventListener('scroll', this.onScroll)
Copy link
Member

@ulivz ulivz Apr 27, 2018

Choose a reason for hiding this comment

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

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

@ulivz
Copy link
Member

ulivz commented Apr 27, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants