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 menu display and remove theme menu #96958

Merged
merged 4 commits into from
May 15, 2022
Merged

Improve settings menu display and remove theme menu #96958

merged 4 commits into from
May 15, 2022

Conversation

GuillaumeGomez
Copy link
Member

We talked about improving the settings menu and we mentioned that firefox pocket was a nice inspiration so I implemented it. The result looks like this:

Screenshot from 2022-05-11 23-59-53

You can test it here.

Only question I have is: should I re-assign the shortcut T to this setting menu now that the theme menu is gone? For now I simply removed it.

Important to be noted: the full settings page (at settings.html) is still rendered the same as currently.

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 11, 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 11, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@GuillaumeGomez GuillaumeGomez force-pushed the settings-menu-display branch from 511b7c7 to 74dd1b0 Compare May 12, 2022 09:04
@GuillaumeGomez
Copy link
Member Author

Fixed eslint errors. :)

jsha
jsha requested changes May 14, 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.

Looks great! The UI interaction works very nicely. All tab selection works as it should, plus opening/closing, clicking outside the dropdown to dismiss, hitting Escape, etc.

The CSS on settings.html is broken in your demo: https://rustdoc.crud.net/imperio/settings-menu-display/doc/settings.html.

Also once this lands we should consider revamping the Help interface to use the same styles. Right now it's weird that the two buttons bring up overlays with two different styles.

src/doc/rustdoc/src/how-to-read-rustdoc.md Outdated Show resolved Hide resolved
src/doc/rustdoc/src/write-documentation/what-to-include.md Outdated Show resolved Hide resolved
src/librustdoc/html/static/css/rustdoc.css Show resolved Hide resolved
src/librustdoc/html/static/js/settings.js Outdated Show resolved Hide resolved
src/librustdoc/html/static/js/settings.js Outdated Show resolved Hide resolved
src/librustdoc/html/static/js/settings.js Outdated Show resolved Hide resolved
@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented May 14, 2022 •
edited
Loading

The CSS on settings.html is broken in your demo: https://rustdoc.crud.net/imperio/settings-menu-display/doc/settings.html.

Indeed, #96939 was merged after. I'll rebase on it so it's all clean. :)

Also once this lands we should consider revamping the Help interface to use the same styles. Right now it's weird that the two buttons bring up overlays with two different styles.

Good idea. :)

@GuillaumeGomez GuillaumeGomez force-pushed the settings-menu-display branch from 74dd1b0 to dee2947 Compare May 14, 2022 15:22
@GuillaumeGomez
Copy link
Member Author

Updated! Also updated the demo.

@jsha
Copy link
Contributor

jsha commented May 14, 2022

@bors r+ rollup

@bors
Copy link
Contributor

bors commented May 14, 2022

📌 Commit dee2947 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 14, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request May 15, 2022
@Dylan-DPC
Rollup merge of rust-lang#96958 - GuillaumeGomez:settings-menu-displa…
cbe1594 
…y, r=jsha

Improve settings menu display and remove theme menu

We talked about improving the settings menu and we mentioned that firefox pocket was a nice inspiration so I implemented it. The result looks like this:

![Screenshot from 2022-05-11 23-59-53](https://user-images.githubusercontent.com/3050060/167954743-438c0a06-4628-478c-bf0c-d20313c1fdfc.png)

You can test it [here](https://rustdoc.crud.net/imperio/settings-menu-display/doc/foo/index.html).

Only question I have is: should I re-assign the shortcut `T` to this setting menu now that the theme menu is gone? For now I simply removed it.

Important to be noted: the full settings page (at `settings.html`) is still rendered the same as currently.

r? `@jsha`
@Dylan-DPC Dylan-DPC mentioned this pull request May 15, 2022
Closed
@matthiaskrgr matthiaskrgr mentioned this pull request May 15, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 15, 2022
@bors
Auto merge of rust-lang#97055 - matthiaskrgr:rollup-1nqwfzx, r=matthi…
e1ec326 
…askrgr

Rollup of 3 pull requests

Successful merges:

 - rust-lang#96958 (Improve settings menu display and remove theme menu)
 - rust-lang#97032 (Allow the unused_macro_rules lint for now)
 - rust-lang#97041 (Fix `download-ci-llvm` NixOS patching for `.so`s.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 7bf43c3 into rust-lang:master May 15, 2022
@rustbot rustbot added this to the 1.63.0 milestone May 15, 2022
@GuillaumeGomez GuillaumeGomez deleted the settings-menu-display branch May 15, 2022 10:24
This was referenced May 15, 2022
Merged
Merged
@notriddle notriddle mentioned this pull request May 25, 2022
Closed
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request May 31, 2022
@Dylan-DPC
Rollup merge of rust-lang#97089 - GuillaumeGomez:improve-settings-the…
94b8036 
…me-display, r=jsha

Improve settings theme display

This is a follow-up of rust-lang#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](https://user-images.githubusercontent.com/3050060/168887703-a01e3bd5-9644-4012-ac11-2ae7bacd6be6.png)
![Screenshot from 2022-05-17 20-46-12](https://user-images.githubusercontent.com/3050060/168887707-132f8b2d-1163-462f-b7dd-f861121bdee7.png)

You can test it [here](https://rustdoc.crud.net/imperio/improve-settings-theme-display/doc/foo/index.html).

r? `@jsha`
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.
6 participants
@GuillaumeGomez @rust-highfive @rust-log-analyzer @jsha @bors @rustbot