-
Notifications
You must be signed in to change notification settings - Fork 391
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
Change content without page refreshing #2727
base: master
Are you sure you want to change the base?
Conversation
ae62b89
to
ce3bc84
Compare
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.
I found a strange bug that is stably reproducible. The first click on the link in the TOC seems to break something, and the page blinks for a second. Then all work as expected.
Screen.Recording.2022-12-15.at.22.16.16.mov
document.querySelectorAll("#sideMenu a") | ||
.forEach(elem => elem.addEventListener('click', (event) => selectPage(event, elem))) | ||
document.querySelectorAll("#main a") | ||
.forEach(elem => elem.addEventListener('click', (event) => selectPage(event, elem))) |
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.
I'd recommend using event delegation than adding an event listener for each element:
- Total memory footprint goes down. It can be significant for an application with many pages in the TOC (StdLib)
- Help prevent memory leaks. With event delegation, you can destroy child elements without the risk of forgetting to "unbind" their event listeners.
- Supports DOM modifications: you can add new elements later, and the event will catch them.
- Simplifies initialization — no need to wait for all elements to be inserted into DOM.
document.querySelectorAll("#sideMenu a") | ||
.forEach(elem => elem.addEventListener('click', (event) => selectPage(event, elem))) | ||
document.querySelectorAll("#main a") | ||
.forEach(elem => elem.addEventListener('click', (event) => selectPage(event, elem))) | ||
}).then(() => { |
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.
Just an advise for code style improvement.
I noticed that there is a chain with many then
functions inside displayNavigationFromPage
function.
The thing is that they useless, only the first does matter. So, all then
blocks can be collapsed to the first one.
document.querySelectorAll("#main a") | ||
.forEach(elem => elem.addEventListener('click', (event) => selectPage(event, elem))) |
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.
I guess these event listeners were initialized before the comment about event delegation was written by me.
Hey, any update on this ? |
#2150