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.

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

Theme detection #1100

Merged
merged 4 commits into from
Oct 22, 2020
Merged

Theme detection #1100

merged 4 commits into from
Oct 22, 2020

Conversation

Nemo157
Copy link
Member

@Nemo157 Nemo157 commented Oct 15, 2020

First step of #918

Moves all the colours out of the style files and uses CSS variables instead, applying a default set on html, then adds a couple of snippets that detect the current rustdoc theme and copies it to a data-theme variable on html, allowing overriding the CSS variables to change the theming.

Next step after this is to add overrides for all the pure specified colours, using CSS variables, so that we can change them; then adding colour sets matching the current dark and ayu rustdoc themes.

@Nemo157
Copy link
Member Author

Nemo157 commented Oct 15, 2020

cc @Cldfire if you have any thoughts

@Cldfire
Copy link
Contributor

Cldfire commented Oct 15, 2020

From my perspective it looks great! The simple variable definitions will make it really easy to add themes. Thank you :)

@jyn514 jyn514 added A-frontend Area: Web frontend S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Oct 15, 2020
Comment on lines +40 to +42
Only other windows get notified when we change local storage, so we have an
invisible iframe that sends us a message when local storage changes so we
can detect rustdoc changing the theme
Copy link
Member

Choose a reason for hiding this comment

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

Only other windows get notified when we change local storage

Who came up with that idea??? 🤦

Well, if that's the case this looks fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, and I was very surprised to find out that neither dedicated or shared workers get the events either, even though they should be treated as a separate context, requiring going all the way to loading another document in an iframe.

templates/style/_navbar.scss Show resolved Hide resolved
templates/style/_themes.scss Outdated Show resolved Hide resolved
html {
--color-background-code: #f5f5f5;
--color-background: #fff;
--color-border-light: lighten(#ddd, 5%);
Copy link
Member

Choose a reason for hiding this comment

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

Should probably be

Suggested change
--color-border-light: lighten(#ddd, 5%);
--color-border-light: lighten(var(--color-border), 5%);

although not sure if that syntax works.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, lighten is an SCSS function which needs the value at compile time, but lucky you mentioned it cause it turns out that function doesn't run in this context for some reason and instead lighten is put into the output CSS, I think we need to just precalculate the values here.

@jyn514 jyn514 added S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Oct 15, 2020
lighten/darken only appear to apply when called from color properties,
not for arbitrary variables.
@Nemo157 Nemo157 added S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed and removed S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Oct 21, 2020
@jyn514 jyn514 merged commit 8abe17a into rust-lang:master Oct 22, 2020
@jyn514
Copy link
Member

jyn514 commented Oct 22, 2020

Thanks for working on this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-frontend Area: Web frontend S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants