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

[rustdoc] Fix system theme detection #63847

Merged
merged 1 commit into from
Aug 31, 2019

Conversation

GuillaumeGomez
Copy link
Member

Fixes #63830

The problem is that it returns the property "entirely" (so with the quotes in our case). Removing them fixes the issue.

cc @fenhl

r? @kinnison

@GuillaumeGomez GuillaumeGomez changed the title Fix system theme detection [rustdoc] Fix system theme detection Aug 24, 2019
@@ -118,7 +118,8 @@ function switchTheme(styleElem, mainStyleElem, newTheme, saveTheme) {
}

function getSystemValue() {
return getComputedStyle(document.documentElement).getPropertyValue('content');
var property = getComputedStyle(document.documentElement).getPropertyValue('content');
return property.replace(/\"/g, "").replace(/\'/g, "");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return property.replace(/\"/g, "").replace(/\'/g, "");
return property.replace(/[\"\']/g, "");

Copy link
Member Author

Choose a reason for hiding this comment

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

In case we change the css for whatever reason, I'd prefer to keep the replace of single quotes as well. Let's avoid surprising regressions if possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Surely @bjorn3's suggestion replaces both double and single quotes, just in one .replace() call?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh indeed! Completely missed it... Sorry @bjorn3...

@kinnison
Copy link
Contributor

I'm afraid I have no way to know for certain if this functions correctly.

@JohnTitor JohnTitor added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 25, 2019
@GuillaumeGomez
Copy link
Member Author

You can run the code of the function in your browser directly (yes, again... sorry...). You just need to ensure it returns "dark" or "light" without double quotes.

@kinnison
Copy link
Contributor

Okay, so with the direct browser intervention I can confirm this works, either the way originally written, or the way that @bjorn3 suggests. I'm fine with either.

@bors r=kinnison

@bors
Copy link
Contributor

bors commented Aug 25, 2019

@kinnison: 🔑 Insufficient privileges: Not in reviewers

@GuillaumeGomez GuillaumeGomez force-pushed the system-theme-detection branch from 4944373 to fcbbf8d Compare August 25, 2019 10:37
@GuillaumeGomez
Copy link
Member Author

Updated with @bjorn3's suggestion.

@fenhl
Copy link
Contributor

fenhl commented Aug 30, 2019

Any updates on this? Do we need to find a different reviewer since @kinnison doesn't have access?

@GuillaumeGomez
Copy link
Member Author

No, I think @kinnison's approval is enough considering this is quite a small PR with a very small impact. I was waiting for CI and forgot about this PR...

@bors: r=kinnison rollup

@bors
Copy link
Contributor

bors commented Aug 30, 2019

📌 Commit fcbbf8d has been approved by kinnison

@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 Aug 30, 2019
Centril added a commit to Centril/rust that referenced this pull request Aug 30, 2019
…on, r=kinnison

[rustdoc] Fix system theme detection

Fixes rust-lang#63830

The problem is that it returns the property "entirely" (so with the quotes in our case). Removing them fixes the issue.

cc @fenhl

r? @kinnison
bors added a commit that referenced this pull request Aug 30, 2019
Rollup of 7 pull requests

Successful merges:

 - #62957 (Match the loop examples)
 - #63600 (Merge oli-obk mail addresses)
 - #63684 (Constify LinkedList new function)
 - #63847 ([rustdoc] Fix system theme detection)
 - #63999 (Add missing links on AsRef trait)
 - #64014 ( miri: detect too large dynamically sized objects )
 - #64015 (some const-eval test tweaks)

Failed merges:

r? @ghost
@bors bors merged commit fcbbf8d into rust-lang:master Aug 31, 2019
@GuillaumeGomez GuillaumeGomez deleted the system-theme-detection branch August 31, 2019 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatic dark theme doesn't work
6 participants