Fix source sidebar bugs #98671
Fix source sidebar bugs #98671
Conversation
GuillaumeGomez
added
T-rustdoc
rust-highfive
added
the
S-waiting-on-review
|
Can you put together a sample with enough source files to make it scroll? |
|
I pushed my local std docs. I updated the link as well. |
|
It works, and it’s definitely an improvement, but it still inherits a long-standing bug where the sidebar doesn’t prevent the main content view from scrolling. Think you can address this one at the same time, or should that be a separate pull request? |
|
I intended it to be this way (to be able to scroll while having the sidebar open on mobile). So not a bug (at least for me ^^'). |
|
@GuillaumeGomez On mobile? Sorry that I forgot to mention that I was testing this on a smartphone. Your screenshots looked like mobile screenshots, since there was no content view visible, but I guess you just cropped them. The desktop behavior is fine, but if the viewport is small enough that the sidebar fills the entire screen, scrolling on the main content view should probably be prevented, since it isn't visible. |
|
Oh I see! I never realized it was possible. I can simply make the other content disappear to prevent it. |
|
I recommend reading this CSS-Tricks post, because if someone scrolls down partway, then opens the full-screen "sidebar", then closes it again without clicking over to a different file, you're going to want to preserve their scroll position. And Mobile Safari does not behave identically to Chrome for Android here. |
|
Writing the GUI test will be a nightmare. 😿 |
|
Oh actually not so much, |
|
So long story short, I also updated the demo. Thanks a lot for all the suggestions @notriddle ! |
|
It doesn’t work in Mobile Safari. Your demo site still allows me to scroll the main page while the full-screen “sidebar” is open. |
|
Sorry, I forgot to upload the docs... Please retry. (I was wondering why the css rule I added wasn't there when testing) |
|
I’ve retried, and it still hasn’t fixed it. Sorry. |
|
So visibility doesn't do the trick... Well, let's try something else then... |
GuillaumeGomez
force-pushed
the
source-sidebar-fixes
branch
from
57d7a1a to
9a1f52d
Compare
|
I updated the fix. I had to include JS (following the tutorial you provided) and it's now working as expected on Android Firefox/Chrome/Opera. I updated the online demo too. |
|
Yes, that works. Thank you. r=me once CI passes |
|
@bors r=notriddle |
|
📌 Commit 9a1f52d has been approved by |
bors
added
S-waiting-on-bors
|
@bors rollup |
…, r=notriddle Fix source sidebar bugs This PR fixes the following two bugs:   I added regression tests to prevent them to happen again. I think we should backport it to beta as well. You can test it [here](https://rustdoc.crud.net/imperio/source-sidebar-fixes/src/std/lib.rs.html). cc `@jsha` r? `@notriddle`
…, r=notriddle Fix source sidebar bugs This PR fixes the following two bugs:   I added regression tests to prevent them to happen again. I think we should backport it to beta as well. You can test it [here](https://rustdoc.crud.net/imperio/source-sidebar-fixes/src/std/lib.rs.html). cc ``@jsha`` r? ``@notriddle``
…askrgr Rollup of 10 pull requests Successful merges: - rust-lang#97629 ([core] add `Exclusive` to sync) - rust-lang#98503 (fix data race in thread::scope) - rust-lang#98670 (llvm-wrapper: adapt for LLVMConstExtractValue removal) - rust-lang#98671 (Fix source sidebar bugs) - rust-lang#98677 (For diagnostic information of Boolean, remind it as use the type: 'bool') - rust-lang#98684 (add test for 72793) - rust-lang#98688 (interpret: add From<&MplaceTy> for PlaceTy) - rust-lang#98695 (use "or pattern") - rust-lang#98709 (Remove unneeded methods declaration for old web browsers) - rust-lang#98717 (get rid of tidy 'unnecessarily ignored' warnings) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
This PR fixes the following two bugs:
I added regression tests to prevent them to happen again.
I think we should backport it to beta as well.
You can test it here.
cc @jsha
r? @notriddle