Fix website history interactions #16060
Conversation
rustbot
added
the
S-waiting-on-review
|
rustbot has assigned @samueltardieu. Use |
|
Thanks, I'll wait to see if @GuillaumeGomez has something to say about this before merging it. Cursory testing indicates that it seems to work as expected. |
| document.addEventListener("keypress", handleShortcut); | ||
| document.addEventListener("keydown", handleShortcut); | ||
|
|
||
| document.querySelectorAll(".dropdown").forEach(setupDropdown); |
There was a problem hiding this comment.
Please use onEachLazy instead of forEach. Chrome recomputes the list at every change made to the DOM.
There was a problem hiding this comment.
A NodeList can be live but forEach doesn't subscribe to changes or anything like that, it's equivalent to Array.prototype.forEach.call(list, f), they are defined to be the same function
> Array.prototype.forEach === NodeList.prototype.forEach
truequerySelectorAll returns a non-live NodeList, additions/removals to the DOM are not reflected in the list like they are with e.g. childNodes. But in this case it would be fine if it were live too, the main issue to look out for with live collections is mutating the DOM while iterating
There was a problem hiding this comment.
A
NodeListcan be live butforEachdoesn't subscribe to changes or anything like that, it's equivalent toArray.prototype.forEach.call(list, f), they are defined to be the same function
Didn't know that, thanks for the information!
| <button class="reset-none">None</button> {# #} | ||
| </li> {# #} | ||
| <li role="separator" class="divider"></li> {# #} | ||
| {% for group in ["cargo", "complexity", "correctness", "nursery", "pedantic", "perf", "restriction", "style", "suspicious", "deprecated"] %} |
There was a problem hiding this comment.
I moved this code on purpose in JS to reduce the page size and also to make it empty in case user's JS is disabled.
There was a problem hiding this comment.
Ah yeah this change wasn't functionally required I just got annoyed at the HTML being split across files when locating class names etc
There's no difference to the page with JS disabled though since you can't expand the dropdowns without JS
It does add some size to the HTML, but they're mostly the same so they compress plenty well. Overall size wise (gzipped) this PR is +246 bytes for the HTML and -1255 bytes for the JS
Up to you @samueltardieu
There was a problem hiding this comment.
There's no difference to the page with JS disabled though since you can't expand the dropdowns without JS
That's part of the things I'd like to change. So better avoid doing back and forth for the same thing. ^^'
It's not really a blocker though. Just more diff in the future when I'm back to this.
There was a problem hiding this comment.
Up to you @samueltardieu
If this is easy not to include this non-required change, then I would indeed prefer not to step on @GuillaumeGomez's ongoing cleanup effort.
There was a problem hiding this comment.
It's some manual effort but yeah revertible, I'll take one more crack at persuasion first to try and avoid it though 😄
If we hide the filter section it's not a problem if they become expandable without JS, it was meant to be hidden in the JS-less page but isn't because of a typo, with that fixed it would no longer matter
There was a problem hiding this comment.
That's a lot of effort to fix a typo 😆
| <button class="reset-none">None</button> {# #} | ||
| </li> {# #} | ||
| <li role="separator" class="divider"></li> {# #} | ||
| {% for level in ["allow", "warn", "deny", "none"] %} |
Alexendoo
force-pushed
the
website-history
branch
from
ed64a76 to
4e35148
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
4 participants
cc @GuillaumeGomez
?search=fooinstead of#/foo, not a breaking change since this didn't work beforeThe bulk of the changes are to support that last one, previously the filter state was stored both in the DOM and as JS objects. The DOM is now the single source of truth
changelog: none