Skip to content

Improve settings theme display - #97089

Merged
bors merged 3 commits into
rust-lang:masterfrom
GuillaumeGomez:improve-settings-theme-display
May 31, 2022
Merged

Improve settings theme display#97089
bors merged 3 commits into
rust-lang:masterfrom
GuillaumeGomez:improve-settings-theme-display

Conversation

@GuillaumeGomez

@GuillaumeGomezGuillaumeGomez commented May 16, 2022•
edited
Loading

Copy link
Copy Markdown
Member

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

@GuillaumeGomezGuillaumeGomez 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

rust-highfive commented May 16, 2022

Copy link
Copy Markdown
Contributor

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez,@Folyd,@jsha

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

notriddle commented May 16, 2022

Copy link
Copy Markdown
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

GuillaumeGomez commented May 16, 2022•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

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
GuillaumeGomezforce-pushed the improve-settings-theme-display branch from 4478860 to d800cdcCompareMay 17, 2022 16:09
@GuillaumeGomez

GuillaumeGomez commented May 17, 2022

Copy link
Copy Markdown
MemberAuthor

It now looks like this:

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

I also updated the demo.

jsha
jsha suggested changes May 17, 2022
View reviewed changes

@jshajsha left a comment

Copy link
Copy Markdown
Contributor

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?

Comment threadsrc/librustdoc/html/static/css/settings.css Outdated
Comment threadsrc/librustdoc/html/static/css/themes/dark.css Outdated
Comment threadsrc/librustdoc/html/static/css/settings.css Outdated
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@GuillaumeGomez
GuillaumeGomezforce-pushed the improve-settings-theme-display branch from d800cdc to ec036afCompareMay 17, 2022 18:42
@GuillaumeGomez

GuillaumeGomez commented May 17, 2022

Copy link
Copy Markdown
MemberAuthor

Updated!

@GuillaumeGomez

GuillaumeGomez commented May 18, 2022

Copy link
Copy Markdown
MemberAuthor

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

@bors

bors commented May 26, 2022

Copy link
Copy Markdown
Collaborator

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

@GuillaumeGomez
GuillaumeGomezforce-pushed the improve-settings-theme-display branch from f1b069a to 35a94ddCompareMay 26, 2022 13:52
@GuillaumeGomez

GuillaumeGomez commented May 26, 2022

Copy link
Copy Markdown
MemberAuthor

Fixed merge conflicts.

jsha
jsha suggested changes May 29, 2022
View reviewed changes

@jshajsha left a comment

Copy link
Copy Markdown
Contributor

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.

Comment threadsrc/librustdoc/html/static/css/settings.css Outdated
Comment threadsrc/librustdoc/html/static/css/settings.css Outdated
@GuillaumeGomez
GuillaumeGomezforce-pushed the improve-settings-theme-display branch from 35a94dd to 70db59cCompareMay 30, 2022 10:15
@GuillaumeGomez

GuillaumeGomez commented May 30, 2022•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

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

@jsha

jsha commented May 30, 2022

Copy link
Copy Markdown
Contributor

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

image

@GuillaumeGomez

GuillaumeGomez commented May 30, 2022

Copy link
Copy Markdown
MemberAuthor

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

jsha commented May 31, 2022

Copy link
Copy Markdown
Contributor

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

@bors r+ rollup

@bors

bors commented May 31, 2022

Copy link
Copy Markdown
Collaborator

📌 Commit 70db59c has been approved by jsha

@borsbors 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-DPCDylan-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 ofrust-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:masterMay 31, 2022
@rustbotrustbot 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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@jshajshajsha requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

@jshajsha

Labels

A-rustdoc-jsArea: Rustdoc's JS front-endA-rustdoc-uiArea: Rustdoc UI (generated HTML)S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-rustdocRelevant 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