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 settings: use radio buttons for theme #93251

Merged
merged 1 commit into from
Jan 25, 2022
Merged

rustdoc settings: use radio buttons for theme #93251

merged 1 commit into from
Jan 25, 2022

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Jan 23, 2022

This reduces the number of clicks required to change theme.

Also, simplify the UI a bit (remove setting grouping), and add a "Back" link close to the settings icon.

Demo: https://rustdoc.crud.net/jsha/theme-radio/settings.html

r? @GuillaumeGomez

New:

image

Old:

image

@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jan 23, 2022
@jsha jsha added the A-rustdoc-ui Area: Rustdoc UI (generated HTML) label Jan 23, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jsha
rustdoc settings: use radio buttons for theme
11b17c6 
This reduces the number of clicks required to change theme.

Also, simplify the UI a bit (remove setting grouping), and add a "Back"
link close to the settings icon.
@GuillaumeGomez
Copy link
Member

It looks super nice! I have now two remarks/questions:

  1. Wouldn't it be better to have theme as a "section title" so we can put the radio buttons below it?
  2. The 1. would probably fix this case on mobile:
    Screenshot from 2022-01-24 10-18-44

@jsha
Copy link
Contributor Author

jsha commented Jan 24, 2022

On desktop, it's nice to have the theme buttons aligned to the right side of the page (like the dropdowns currently are), so it's a short trip from the settings icon to the theme buttons.

Also, I wanted to make sure to have flex-wrap: wrap to cover the (unlikely) case when someone adds a lot of custom themes.

I tried to reproduce the screenshot you showed, and the wrapping doesn't show up until the screen is narrower than 317px. I think that's such a rare screen size that it's okay if the buttons wrap.

@GuillaumeGomez
Copy link
Member

I find it hard on desktop to see that it's from the setting "themes" actually since there is no visual "helper".

@GuillaumeGomez
Copy link
Member

After a discussion, we decided to first land this change and to make a bigger refactoring on the settings menu overall.

@GuillaumeGomez
Copy link
Member

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Jan 24, 2022

📌 Commit 11b17c6 has been approved by GuillaumeGomez

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jan 24, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 25, 2022
@matthiaskrgr
Rollup merge of rust-lang#93251 - jsha:theme-radio, r=GuillaumeGomez
691bc38 
rustdoc settings: use radio buttons for theme

This reduces the number of clicks required to change theme.

Also, simplify the UI a bit (remove setting grouping), and add a "Back" link close to the settings icon.

Demo: https://rustdoc.crud.net/jsha/theme-radio/settings.html

r? `@GuillaumeGomez`

New:

![image](https://user-images.githubusercontent.com/220205/150702647-4826d525-54fa-439a-b24c-6d5bca6f95bf.png)

Old:

![image](https://user-images.githubusercontent.com/220205/150702669-6a4214ed-1dab-4fee-b1aa-59acfce3dbca.png)
This was referenced Jan 25, 2022
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 25, 2022
@bors
Auto merge of rust-lang#93288 - matthiaskrgr:rollup-uu4uwd1, r=matthi…
df368ae 
…askrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#88794 (Add a `try_clone()` function to `OwnedFd`.)
 - rust-lang#93064 (Properly track `DepNode`s in trait evaluation provisional cache)
 - rust-lang#93118 (Move param count error emission to end of `check_argument_types`)
 - rust-lang#93144 (Work around missing code coverage data causing llvm-cov failures)
 - rust-lang#93169 (Fix inconsistency of local blanket impls)
 - rust-lang#93175 (Implement stable overlap check considering negative traits)
 - rust-lang#93251 (rustdoc settings: use radio buttons for theme)
 - rust-lang#93269 (Use error-on-mismatch policy for PAuth module flags.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c3ddca6 into rust-lang:master Jan 25, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 25, 2022
@camelid
Copy link
Member

camelid commented Jan 25, 2022

I think this PR broke the display of the search bar:

beta:

image

nightly:

image

Notice how the padding and border-radius are wrong.

@GuillaumeGomez
Copy link
Member

@camelid Do you mean this one instead?

@camelid
Copy link
Member

camelid commented Jan 25, 2022

Ah, perhaps. The old and new screenshots in this PR's description show a change in the search box size, which misled me.

@GuillaumeGomez
Copy link
Member

Ah true. Well, it was a change done on purpose in any case. ;)

@notriddle notriddle mentioned this pull request May 16, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
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.60.0
Development

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