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.

Sign up for GitHub

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

Jump to bottom

rustdoc: click target for sidebar items flush left #127229

Merged
merged 3 commits into from
Jul 18, 2024
Merged

rustdoc: click target for sidebar items flush left #127229

merged 3 commits into from
Jul 18, 2024

Conversation

notriddle
Copy link
Contributor

@notriddle notriddle commented Jul 2, 2024 •
edited
Loading

This change adjusts the clickable area of sidebar links to touch the leftmost edge of the canvas, making them much easier to click (when the browser window is maximized or tiled left, but those cases are common enough to matter).

Screencast.from.2024-07-15.15-31-07.webm
old screencast
Screencast.from.2024-07-01.17-23-34.webm

@notriddle notriddle added the T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. label Jul 2, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jul 2, 2024

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot
Copy link
Collaborator

rustbot commented Jul 2, 2024

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @jsha

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 2, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jul 2, 2024

Some changes occurred in GUI tests.

cc @GuillaumeGomez

@GuillaumeGomez
Copy link
Member

Just one question: should we also extend the background to the left? Makes it a bit weird because without knowing that this will click the link, I'm not sure if it's not gonna click something else.

@notriddle
Copy link
Contributor Author

I wouldn’t mind extending the background all the way to the left. It’s actually easier to do it that way.

@GuillaumeGomez
Copy link
Member

Even better then. I'll approve once done.

@notriddle
Copy link
Contributor Author

Wait, now I know why that doesn't work.

image

The logo lockup relies on the background not extending past where the text is. z-index might be used to avoid covering up the image, but it would still look weird (since the image is transparent, you'd be able to see the background extend under it).

@GuillaumeGomez
Copy link
Member 

.logo-container + h2 > a {
// current style
}

should do the trick?

@fmease fmease assigned GuillaumeGomez and unassigned fmease Jul 2, 2024
@notriddle
Copy link
Contributor Author

The problem isn't that I want to make it different. It's that I want it to be the same.

Screen.Recording.2024-07-02.at.08.40.15.mov

If the text is long enough, it line wraps. When that happens, I want it to look and act just like any other nav item. If the other ones have a background that goes all the way to the edge, then this one should, too.

@GuillaumeGomez
Copy link
Member

I see. Urg, UI.

@notriddle
Copy link
Contributor Author

@GuillaumeGomez

I've figured out a reasonable way to make it work right with the logo lockup. It shows the highlight going all the way to the left for all menu items, including the crate name, except when it's in a horizontal lockup.

Screencast.from.2024-07-15.15-31-07.webm

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@GuillaumeGomez
Copy link
Member

Wow, impressive, well done!

Please add a GUI test then I'll make one last review before r+.

@notriddle
Copy link
Contributor Author

@GuillaumeGomez Sure, here's a test case for you.

GuillaumeGomez
GuillaumeGomez reviewed Jul 16, 2024
View reviewed changes
tests/rustdoc-gui/sidebar.goml
// Check that the sidebar links touch the left side of the box
go-to: "file://" + |DOC_PATH| + "/test_docs/index.html"
assert-position: (".sidebar .block a", {"x": -4})
assert-position: (".sidebar-crate > h2 > a", {"x": -3})
Copy link
Member

Choose a reason for hiding this comment

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

Could you also test the resizing (where the image move)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. I've updated that test case here.

Copy link
Member

Choose a reason for hiding this comment

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

Ah sorry, wasn't clear enough. For both cases, can you resize the side to ensure that the text moves under the logo when it's too small please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lib2 doesn't have a logo at all, so resizing won't do anything.

huge_logo does have a logo, so I've added another test.

Copy link
Member

Choose a reason for hiding this comment

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

👍

@GuillaumeGomez
Copy link
Member

It's great, thanks a lot!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 17, 2024

📌 Commit 5514906 has been approved by GuillaumeGomez

It is now in the queue for this repository.

@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 17, 2024
@matthiaskrgr matthiaskrgr mentioned this pull request Jul 17, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 17, 2024
@bors
Auto merge of rust-lang#127865 - matthiaskrgr:rollup-8m49dlg, r=matth…
c9517f3 
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#125042 (Use ordinal number in argument error)
 - rust-lang#127229 (rustdoc: click target for sidebar items flush left)
 - rust-lang#127337 (Move a few intrinsics to Rust abi)
 - rust-lang#127472 (MIR building: Stop using `unpack!` for `BlockAnd<()>`)
 - rust-lang#127579 (Solve a error `.clone()` suggestion when moving a mutable reference)
 - rust-lang#127769 (Don't use implicit features in `Cargo.toml` in `compiler/`)
 - rust-lang#127844 (Remove invalid further restricting suggestion for type bound)
 - rust-lang#127855 (Add myself to review rotation)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 17, 2024
@bors
Auto merge of rust-lang#127865 - matthiaskrgr:rollup-8m49dlg, r=matth…
798d1eb 
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#125042 (Use ordinal number in argument error)
 - rust-lang#127229 (rustdoc: click target for sidebar items flush left)
 - rust-lang#127337 (Move a few intrinsics to Rust abi)
 - rust-lang#127472 (MIR building: Stop using `unpack!` for `BlockAnd<()>`)
 - rust-lang#127579 (Solve a error `.clone()` suggestion when moving a mutable reference)
 - rust-lang#127769 (Don't use implicit features in `Cargo.toml` in `compiler/`)
 - rust-lang#127844 (Remove invalid further restricting suggestion for type bound)
 - rust-lang#127855 (Add myself to review rotation)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 17, 2024
@bors
Auto merge of rust-lang#127865 - matthiaskrgr:rollup-8m49dlg, r=matth…
e35364a 
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#125042 (Use ordinal number in argument error)
 - rust-lang#127229 (rustdoc: click target for sidebar items flush left)
 - rust-lang#127337 (Move a few intrinsics to Rust abi)
 - rust-lang#127472 (MIR building: Stop using `unpack!` for `BlockAnd<()>`)
 - rust-lang#127579 (Solve a error `.clone()` suggestion when moving a mutable reference)
 - rust-lang#127769 (Don't use implicit features in `Cargo.toml` in `compiler/`)
 - rust-lang#127844 (Remove invalid further restricting suggestion for type bound)
 - rust-lang#127855 (Add myself to review rotation)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0472b2a into rust-lang:master Jul 18, 2024
6 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jul 18, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jul 18, 2024
@rust-timer
Unrolled build for rust-lang#127229
4729e30 
Rollup merge of rust-lang#127229 - notriddle:notriddle/mile-wide-bar, r=GuillaumeGomez

rustdoc: click target for sidebar items flush left

This change adjusts the clickable area of sidebar links to touch the leftmost edge of the canvas, making them [much easier](https://www.nngroup.com/articles/fitts-law/) to click (when the browser window is maximized or tiled left, but those cases are common enough to matter).

[Screencast from 2024-07-15 15-31-07.webm](https://github.com/user-attachments/assets/1e952d3a-e9e7-476b-b211-44a17c190b38)

<details><summary>old screencast</summary>

[Screencast from 2024-07-01 17-23-34.webm](https://github.com/rust-lang/rust/assets/1593513/dc6f9c2e-5904-403d-b353-d233e6e1afbc)

</details>
@notriddle notriddle deleted the notriddle/mile-wide-bar branch July 18, 2024 06:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuillaumeGomez GuillaumeGomez GuillaumeGomez approved these changes

Assignees

@GuillaumeGomez GuillaumeGomez

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output.
Projects
None yet
Milestone
1.81.0
Development

Successfully merging this pull request may close these issues.
6 participants
@notriddle @rustbot @GuillaumeGomez @rust-log-analyzer @bors @fmease