Skip to content

[rustdoc] Fix system theme detection#63847

Merged
bors merged 1 commit intorust-lang:masterfrom
GuillaumeGomez:system-theme-detection
Aug 31, 2019
Merged

[rustdoc] Fix system theme detection#63847
bors merged 1 commit intorust-lang:masterfrom
GuillaumeGomez:system-theme-detection

Conversation

@GuillaumeGomez
Copy link
Copy Markdown
Member

@GuillaumeGomez GuillaumeGomez commented Aug 24, 2019

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
bjorn3
bjorn3 reviewed Aug 24, 2019
View reviewed changes
src/librustdoc/html/static/storage.js Outdated
Copy link
Copy Markdown
Member

@bjorn3 bjorn3 Aug 24, 2019

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
Copy Markdown
Member Author

@GuillaumeGomez GuillaumeGomez Aug 25, 2019

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
Copy Markdown
Contributor

@kinnison kinnison Aug 25, 2019

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
Copy Markdown
Member Author

@GuillaumeGomez GuillaumeGomez Aug 25, 2019

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
Copy Markdown
Contributor

kinnison commented Aug 24, 2019

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
Copy Markdown
Member Author

GuillaumeGomez commented Aug 25, 2019

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
Copy Markdown
Contributor

kinnison commented Aug 25, 2019

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
Copy Markdown
Collaborator

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
Copy Markdown
Member Author

GuillaumeGomez commented Aug 25, 2019

Updated with @bjorn3's suggestion.

@fenhl
Copy link
Copy Markdown
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
Copy Markdown
Member Author

GuillaumeGomez commented Aug 30, 2019

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
Copy Markdown
Collaborator

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
@Centril
Rollup merge of rust-lang#63847 - GuillaumeGomez:system-theme-detecti…
5f07ff7 
…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
@Centril Centril mentioned this pull request Aug 30, 2019
Merged
bors added a commit that referenced this pull request Aug 30, 2019
@bors
Auto merge of #64026 - Centril:rollup-le667lp, r=Centril
fd68d02 
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
@fenhl fenhl mentioned this pull request Sep 1, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bjorn3 bjorn3 bjorn3 left review comments

+1 more reviewer

@kinnison kinnison kinnison left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Automatic dark theme doesn't work

6 participants

@GuillaumeGomez @kinnison @bors @fenhl @bjorn3 @JohnTitor