Skip to content
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

rustdoc: improve click behavior of the source code mobile full-screen "sidebar" #98776

Merged

Conversation

notriddle
Copy link
Contributor

On desktop, if you open the source code sidebar, it stays open even when you move from page to page. It used to do the same thing on mobile, but I think that's stupid. Since the file list fills the entire screen on mobile, and you can't really do anything with the currently selected file other than dismiss the "sidebar" to look at it, it's safe to assume that anybody who clicks a file in that list probably wants the list to go away so they can see it.

Split out separately from #98772

@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jul 1, 2022
@rustbot
Copy link
Collaborator

rustbot commented Jul 1, 2022

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @Folyd, @jsha

@rust-highfive
Copy link
Collaborator

r? @CraftSpider

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 1, 2022
@notriddle notriddle changed the title Improve click behavior of the source code mobile full-screen "sidebar" rustdoc: improve click behavior of the source code mobile full-screen "sidebar" Jul 1, 2022
@rust-log-analyzer

This comment has been minimized.

@notriddle notriddle force-pushed the notriddle/mobile-sidebar-auto-close branch 2 times, most recently from 9ae57a4 to d7141da Compare July 1, 2022 20:28
@@ -48,6 +54,7 @@ function createDirEntry(elem, parent, fullPath, hasFoundFile) {
const file = document.createElement("a");
file.innerText = file_text;
file.href = rootPath + "src/" + fullPath + file_text + ".html";
file.addEventListener("click", closeSidebarIfMobile);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of doing this, wouldn't it be simpler to not open the sidebar by default when we arrive on a new page with a "mobile width"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be possible, but it would open up the following, very weird timeline:

  1. A user with a 7-inch Android tablet in portrait mode clicks open the sidebar. The sidebar is now open, and rustdoc-source-sidebar-show == "true".
  2. The user clicks a link. The sidebar is now closed, but rustdoc-source-sidebar-show == "true".
  3. The user rotates their tablet into landscape mode. The sidebar is still closed, but rustdoc-source-sidebar-show == "true".
  4. The user clicks an inline jump to definition link.
  5. The sidebar pops open, seemingly out of nowhere. This is bad.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense indeed. Can you add a test where we check that a "resize" in-between won't re-open the sidebar as you mentioned?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I've pushed commit 6e2c49f adding a test case for this.

@GuillaumeGomez
Copy link
Member

This is a very good point! I think we can simplify it a bit by just not opening the sidebar when we arrive on a new page and we have a "mobile width".

On desktop, if you open the source code sidebar, it stays open even when you
move from page to page. It used to do the same thing on mobile, but I think
that's stupid. Since the file list fills the entire screen on mobile, and you
can't really do anything with the currently selected file other than dismiss
the "sidebar" to look at it, it's safe to assume that anybody who clicks a
file in that list probably wants the list to go away so they can see it.
@notriddle notriddle force-pushed the notriddle/mobile-sidebar-auto-close branch from d7141da to 83f2288 Compare July 1, 2022 21:33
@GuillaumeGomez
Copy link
Member

Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 5, 2022

📌 Commit 6e2c49f has been approved by GuillaumeGomez

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 5, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 5, 2022
Rollup of 5 pull requests

Successful merges:

 - rust-lang#97712 (ptr::copy and ptr::swap are doing untyped copies)
 - rust-lang#98624 (lints: mostly translatable diagnostics)
 - rust-lang#98776 (rustdoc: improve click behavior of the source code mobile full-screen "sidebar")
 - rust-lang#98856 (Remove FIXME from rustdoc intra-doc test)
 - rust-lang#98913 (:arrow_up: rust-analyzer)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c2613a5 into rust-lang:master Jul 5, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 5, 2022
@notriddle notriddle deleted the notriddle/mobile-sidebar-auto-close branch July 5, 2022 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants