-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
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
[docs-infra] Simplify click header #42593
[docs-infra] Simplify click header #42593
Conversation
Netlify deploy previewhttps://deploy-preview-42593--material-ui.netlify.app/ Bundle size report |
const elements = document.getElementsByClassName('title-link-to-anchor'); | ||
const elements = rootRef.current!.getElementsByClassName('title-link-to-anchor'); |
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.
The CSS is scoped to the component, so much better not to leak outside of this scope, especially since we bypass React here.
elements[i].addEventListener('click', handleClick, false); | ||
elements[i].addEventListener('click', handleHeaderClick); |
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.
- false is the default argument, no need
- false is missing from the remove event handler, fix the removal logic
handleHeaderClick
feels clearer
|
||
return () => { | ||
for (let i = 0; i < elements.length; i += 1) { | ||
elements[i].removeEventListener('click', handleClick); | ||
} | ||
}; |
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.
If the component unmounts, these DOM nodes are removed. I imagine we don't need to remove event handlers. Even if the content is dynamic, removed DOM nodes would be gone, and elements[i].addEventListener('click',
is smart, it deduplicates listeners if the arguments are identical (https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener), which is the case since handleClick
is hoisted to the top level scope.
|
||
if (isRangeSelection) { | ||
if (selection.type === 'Range') { | ||
// Disable the <a> behavior to be able to select text. |
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.
We have this implicit rule to try to document each event.preventDefault();
call in the source because they tend to have strong implications, it's hard to not break stuff with them.
08b77c4
to
44f05fa
Compare
A follow-up on #41994, trying to polish the edges.
Preview: https://deploy-preview-42593--material-ui.netlify.app/material-ui/getting-started/