-
Notifications
You must be signed in to change notification settings - Fork 13.8k
Improve settings theme display #97089
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
Improve settings theme display #97089
Conversation
Some changes occurred in HTML/CSS/JS.
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.
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.
4478860 to
d800cdc
Compare
There was a problem hiding this 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?
This comment has been minimized.
This comment has been minimized.
d800cdc to
ec036af
Compare
Updated!
I moved the "color" CSS rules into the theme files where they should always have been.
☔ The latest upstream changes (presumably #97409) made this pull request unmergeable. Please resolve the merge conflicts.
f1b069a to
35a94dd
Compare
Fixed merge conflicts.
There was a problem hiding this 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.
35a94dd to
70db59c
Compare
Updated PR and the demo (and added a GUI test as well)!
The latest demo looks quite messed up (Chrome, Ubuntu, normal-width browser):
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.
I can't reproduce the bug either. Looks good!
@bors r+ rollup
📌 Commit 70db59c has been approved by jsha
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
Uh oh!
There was an error while loading. Please reload this page.
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