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

Improve settings theme display #97089

Merged
merged 3 commits into from
May 31, 2022
Merged

Improve settings theme display #97089

merged 3 commits into from
May 31, 2022

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented May 16, 2022 •
edited
Loading

This is a follow-up of #96958. In this PR, I changed how the theme radio buttons are displayed and improved their look as well.

It now looks like this:

Screenshot from 2022-05-17 20-46-20
Screenshot from 2022-05-17 20-46-12

You can test it here.

r? @jsha

@GuillaumeGomez GuillaumeGomez added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-rustdoc-ui Area: Rustdoc UI (generated HTML) A-rustdoc-js Area: Rustdoc's JS front-end labels May 16, 2022
@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez,@Folyd,@jsha

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 16, 2022
@notriddle
Copy link
Contributor

rustdoc used to use a <select> menu for settings. It was switched to radio buttons to reduce the number of clicks. I'd rather not flip-flop unless we can address the original rationale. #93251

If you're just trying to make the UI more consistent and simple, is there a reason why making the radio buttons always show up on their own line won't satisfy it? I know that they were originally aligned right to shorten the distance needed to move the mouse, but now that the settings are shown in a popover, the distance won't be very long no matter how big the browser window is.

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented May 16, 2022 •
edited
Loading

We were talking about it with @jsha. I think I'd be fine with radio buttons on their own line (I really don't like having them where they are located now). Also the brush image will be removed (we kinda all agree on this part hehe).

But in any case, needs to discuss things before doing anything in here.

@GuillaumeGomez GuillaumeGomez force-pushed the improve-settings-theme-display branch from 4478860 to d800cdc Compare May 17, 2022 16:09
@GuillaumeGomez
Copy link
Member Author

It now looks like this:

Screenshot from 2022-05-17 18-10-12

I also updated the demo.

jsha
jsha requested changes May 17, 2022
View reviewed changes
Copy link
Contributor

@jsha jsha left a comment

Choose a reason for hiding this comment

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

This looks nice! The larger radio buttons are a big improvement, and removing the borders around the labels is elegant.

Can you update the PR description with an up to date description and screenshot?

src/librustdoc/html/static/css/settings.css Outdated Show resolved Hide resolved
src/librustdoc/html/static/css/themes/dark.css Show resolved Hide resolved
src/librustdoc/html/static/css/settings.css Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@GuillaumeGomez GuillaumeGomez force-pushed the improve-settings-theme-display branch from d800cdc to ec036af Compare May 17, 2022 18:42
@GuillaumeGomez
Copy link
Member Author

Updated!

@GuillaumeGomez
Copy link
Member Author

I moved the "color" CSS rules into the theme files where they should always have been.

@bors
Copy link
Contributor

bors commented May 26, 2022

☔ The latest upstream changes (presumably #97409) made this pull request unmergeable. Please resolve the merge conflicts.

@GuillaumeGomez GuillaumeGomez force-pushed the improve-settings-theme-display branch from f1b069a to 35a94dd Compare May 26, 2022 13:52
@GuillaumeGomez
Copy link
Member Author

Fixed merge conflicts.

jsha
jsha requested changes May 29, 2022
View reviewed changes
Copy link
Contributor

@jsha jsha left a comment

Choose a reason for hiding this comment

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

There's a regression with tab navigation: If you tab through the items in the settings menu, no outline appears when you have selected the radio buttons.

src/librustdoc/html/static/css/settings.css Outdated Show resolved Hide resolved
src/librustdoc/html/static/css/settings.css Show resolved Hide resolved
@GuillaumeGomez GuillaumeGomez force-pushed the improve-settings-theme-display branch from 35a94dd to 70db59c Compare May 30, 2022 10:15
@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented May 30, 2022 •
edited
Loading

Updated PR and the demo (and added a GUI test as well)!

@jsha
Copy link
Contributor

jsha commented May 30, 2022

The latest demo looks quite messed up (Chrome, Ubuntu, normal-width browser):

image

@GuillaumeGomez
Copy link
Member Author

I cannot reproduce your bug. While looking for it, I saw another one for very small width screen so I fixed it (needed to add a min-width for .slider). I pushed and updated the demo as well.

@jsha
Copy link
Contributor

jsha commented May 31, 2022

I can't reproduce the bug either. Looks good!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented May 31, 2022

📌 Commit 70db59c has been approved by jsha

@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 May 31, 2022
@Dylan-DPC Dylan-DPC mentioned this pull request May 31, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 31, 2022
@bors
Auto merge of rust-lang#97574 - Dylan-DPC:rollup-jq850l6, r=Dylan-DPC
d35d972 
Rollup of 6 pull requests

Successful merges:

 - rust-lang#97089 (Improve settings theme display)
 - rust-lang#97229 (Document the current aliasing rules for `Box<T>`.)
 - rust-lang#97371 (Suggest adding a semicolon to a closure without block)
 - rust-lang#97455 (Stabilize `toowned_clone_into`)
 - rust-lang#97565 (Add doc alias `memset` to `write_bytes`)
 - rust-lang#97569 (Remove `memset` alias from `fill_with`.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 94b8036 into rust-lang:master May 31, 2022
@rustbot rustbot added this to the 1.63.0 milestone May 31, 2022
@GuillaumeGomez GuillaumeGomez deleted the improve-settings-theme-display branch May 31, 2022 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsha jsha jsha requested changes

Assignees

@jsha jsha

Labels
A-rustdoc-js Area: Rustdoc's JS front-end A-rustdoc-ui Area: Rustdoc UI (generated HTML) 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
Milestone
1.63.0
Development

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