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: mobile nav fixes #93183

Merged
merged 1 commit into from
Jan 30, 2022
Merged

rustdoc: mobile nav fixes #93183

merged 1 commit into from
Jan 30, 2022

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Jan 22, 2022

  • Make sure the mobile-topbar doesn't overflow its height if the user sets a bigger font.

  • Make sure the sidebar can be scrolled all the way to the bottom by shortening it to accommodate the mobile-topbar.

  • Make the item name in the mobile-topbar clickable to go to the top of the page.

  • Remove excess padding sidebar in mobile mode.

Demo https://rustdoc.crud.net/jsha/mobile-nav-fixes/std/string/struct.String.html

r? @GuillaumeGomez

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

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez,@Folyd

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 22, 2022
@GuillaumeGomez
Copy link
Member

Did you encounter this problem somewhere? If so, can you add a GUI test to prevent a regression please?

@jsha
Copy link
Contributor Author

jsha commented Jan 22, 2022 via email

@GuillaumeGomez
Copy link
Member

For the font size issue: is there a way to set the browser's font size in
browser-UI-test?

I actually don't know how to set the browser's font size, so I can't answer this question... I'll check that tomorrow.

@GuillaumeGomez
Copy link
Member

I think this does what you want?

@jsha
Copy link
Contributor Author

jsha commented Jan 25, 2022

I think so? That links to https://vanilla.aslushnikov.com/?Page.setFontSizes, which seems to do what I need. But that needs to be exposed in browser-UI-test, right?

@jsha
Copy link
Contributor Author

jsha commented Jan 25, 2022

Also how do I tell browser-UI-test "scroll this specific container all the way to its bottom?"

@GuillaumeGomez
Copy link
Member

I think so? That links to https://vanilla.aslushnikov.com/?Page.setFontSizes, which seems to do what I need. But that needs to be exposed in browser-UI-test, right?

I need to add it indeed.

Also how do I tell browser-UI-test "scroll this specific container all the way to its bottom?"

Just to be sure: do you mean like scrolling to the end of a text field or really going to the bottom of a given element? For the second, you can always scroll to the element just after it. Or we could add a parameter/new command in browser-ui-test?

In both cases, can you open issues there please so I can put them in my todo list?

@jsha jsha force-pushed the mobile-nav-fixes branch from e448be6 to 7c24468 Compare January 26, 2022 00:21
@jsha
Copy link
Contributor Author

jsha commented Jan 26, 2022

Filed GuillaumeGomez/browser-UI-test#248 and updated the PR with a small, imperfect test for the scrolling issue.

@GuillaumeGomez
Copy link
Member

I found a weird case:

Screenshot from 2022-01-27 10-08-20

@jsha
Copy link
Contributor Author

jsha commented Jan 27, 2022

What browser, font size, and screen / viewport dimensions?

@GuillaumeGomez
Copy link
Member

Firefox, default font size and < 420px.

- Make sure the mobile-topbar doesn't overflow its height if the user
  sets a bigger font.

- Make sure the sidebar can be scrolled all the way to the bottom by
  shortening it to accommodate the mobile-topbar.

- Make the item name in the mobile-topbar clickable to go to the top of
  the page.

- Remove excess padding sidebar in mobile mode.
@jsha jsha force-pushed the mobile-nav-fixes branch from 7c24468 to a998a37 Compare January 30, 2022 01:13
@jsha
Copy link
Contributor Author

jsha commented Jan 30, 2022

Found it and pushed a fix! The heading was wrapping, which was causing a scrollbar. Turned off wrapping for that heading, and set overflow: hidden (previous was just overflow-x: hidden).

// Check that the bottom-most item on the sidebar menu can be scrolled fully into view.
click: ".sidebar-menu-toggle"
scroll-to: ".block.keyword li:nth-child(1)"
assert-position: (".block.keyword li:nth-child(1)", {"y": 542.96875})
Copy link
Member

Choose a reason for hiding this comment

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

Wow, that's quite the number. XD

@GuillaumeGomez
Copy link
Member

Thanks!

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Jan 30, 2022

📌 Commit a998a37 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 Jan 30, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 30, 2022
Rollup of 5 pull requests

Successful merges:

 - rust-lang#92887 (Bootstrap compiler update)
 - rust-lang#92908 (Render more readable macro matcher tokens in rustdoc)
 - rust-lang#93183 (rustdoc: mobile nav fixes)
 - rust-lang#93192 (Add VS 2022 into error message)
 - rust-lang#93475 (Add test to ensure that theme is applied correctly when going back in history)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4ddf986 into rust-lang:master Jan 30, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 30, 2022
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.

5 participants