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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bc3336b
add active sidebar link highlight
ryanrivest Apr 26, 2018
30800ac
use router to update hash
ryanrivest Apr 27, 2018
ffdd8b7
update throttle import
ryanrivest Apr 27, 2018
7ca5f5c
chore: remove comments
ulivz Apr 27, 2018
c67c64a
chore: simplify code
ulivz Apr 27, 2018
58f4b02
chore: change throttle duration from 200 to 300
ulivz Apr 27, 2018
1c4c376
chore: tweaks
ulivz Apr 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 38 additions & 1 deletion lib/default-theme/Layout.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.


export default {
components: { Home, Page, Sidebar, Navbar },
Expand Down Expand Up @@ -111,6 +112,8 @@ export default {
this.$watch('$page', updateMeta)
updateMeta()

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

@ulivz ulivz Apr 27, 2018 •
edited
Loading

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 •
edited
Loading

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! 🎉


// configure progress bar
nprogress.configure({ showSpinner: false })

Expand All @@ -129,6 +132,8 @@ export default {

beforeDestroy () {
updateMetaTags(null, this.currentMetaTags)

window.removeEventListener('scroll', this.onScroll)
},

methods: {
Expand All @@ -152,7 +157,10 @@ export default {
this.toggleSidebar(false)
}
}
}
},
onScroll: throttle(() => {
setActiveHash()
}, 200)
}
}

Expand All @@ -173,6 +181,35 @@ function updateMetaTags (meta, current) {
})
}
}

function setActiveHash () {
const anchors = gatherHeaderAnchors()

const scrollTop = Math.max(window.pageYOffset, document.documentElement.scrollTop, document.body.scrollTop)

for (let i = 0; i < anchors.length; i++) {
const anchor = anchors[i]
const nextAnchor = anchors[i + 1]

const isActive = i === 0 && scrollTop === 0 ||
(scrollTop >= anchor.parentElement.offsetTop + 10 &&
(typeof nextAnchor === 'undefined' || scrollTop < nextAnchor.parentElement.offsetTop - 10))

if (isActive && window.location.hash !== anchor.hash) {
window.location.hash = anchor.hash
return
}
}
}

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 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 •
edited
Loading

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


return anchors
}
</script>

<style src="prismjs/themes/prism-tomorrow.css"></style>
Expand Down
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
"koa-mount": "^3.0.0",
"koa-static": "^4.0.2",
"loader-utils": "^1.1.0",
"lodash.throttle": "^4.1.1",
"lru-cache": "^4.1.2",
"markdown-it": "^8.4.1",
"markdown-it-anchor": "^4.0.0",
Expand Down