-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Move global click handlers to per-element ones. #85117
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
Conversation
Some changes occurred in HTML/CSS/JS. |
☔ The latest upstream changes (presumably #85074) made this pull request unmergeable. Please resolve the merge conflicts. |
The source code pages are completely broken too (both the line numbers and the display). You also forgot to update this line: onEachLazy(document.getElementsByClassName("line-numbers"), function(e) { I think a few others might be missing too. |
Also, that might conflict with #85148 |
Thanks for working on this! |
6c26bd5
to
b120726
Compare
Rebased on latest master and fixed the review issues. I also wound up moving the handlers for source line highlighting into source-script.js. Updated the PR description, commit description, and demo. |
@@ -1,3 +1,4 @@ | |||
(function() { |
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.
Nice! Could you move the eslint comments at the top please? Makes it easier to read.
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 one last nit and then it's good to go. Thanks a lot for this improvement!
b120726
to
33d929c
Compare
@@ -105,7 +105,7 @@ crate fn render<T: Print, S: Print>( | |||
placeholder=\"Click or press ‘S’ to search, ‘?’ for more options…\" \ | |||
type=\"search\">\ | |||
</div>\ | |||
<button type=\"button\" class=\"help-button\">?</button> | |||
<button type=\"button\" id=\"help-button\">?</button> |
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.
Please add this ID into init_id_map
(we should really write a tidy check for that, gonna do that tomorrow).
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.
What does the IdMap
do?
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.
When we generate IDs for anchors, we check this map to ensure that the ID doesn't already exists, otherwise, we add a -X
(where X is a number) to avoid duplicates. It's also used (well, at least by me) to check that the ID doesn't already exist when I create a new one in some new HTML part. :)
In rustdoc's main.js, we had an onclick handler for the whole document that would dispatch to handlers for various elements. This change attaches the handlers to the elements that trigger them, instead. This simplfies the code and avoids reimplementing the browser's bubbling functionality. As part of this change, change from a class to an id for help button. Move the handlers and associated code for highlighting source lines into source-script.js (and factor out a shared regex).
33d929c
to
0945451
Compare
Thanks! @bors: r+ |
📌 Commit 0945451 has been approved by |
…laumeGomez Rollup of 5 pull requests Successful merges: - rust-lang#84793 (Recover from invalid `struct` item syntax) - rust-lang#85117 (Move global click handlers to per-element ones.) - rust-lang#85141 (Update documentation for SharedContext::maybe_collapsed_doc_value) - rust-lang#85174 (Fix border radius for doc code blocks in rustdoc) - rust-lang#85205 (Update books) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
In rustdoc's main.js, we had an onclick handler for the whole document that would dispatch to handlers for various elements. This change attaches the handlers to the elements that trigger them, instead. This simplifies the code and avoids reimplementing the browser's bubbling functionality.
As part of this change, change from a class to an id for help button.
Move the handlers and associated code for highlighting source lines into source-script.js (and factor out a shared regex).
Demo at https://hoffman-andrews.com/rust/bubble-bubble-toil-and-trouble/std/string/struct.String.html
Note: this conflicts with / depends on #85074. Once that's merged I'll rebase this and resolve conflicts.
Part of #83332. Thanks to @Manishearth for the suggestion to not reimplement bubbling.
r? @GuillaumeGomez