[docs-infra] Simplify click header#42593
Conversation
oliviertassinari
added
internal
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.
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.
- false is the default argument, no need
- false is missing from the remove event handler, fix the removal logic
handleHeaderClickfeels clearer
|
|
||
| return () => { | ||
| for (let i = 0; i < elements.length; i += 1) { | ||
| elements[i].removeEventListener('click', handleClick); | ||
| } | ||
| }; |
There was a problem hiding this comment.
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.
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.
oliviertassinari
force-pushed
the
simplify-click-solution
branch
from
08b77c4 to
44f05fa
Compare
3 participants
A follow-up on #41994, trying to polish the edges.
Preview: https://deploy-preview-42593--material-ui.netlify.app/material-ui/getting-started/