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

fix(v2): set stored theme only if it exists #2113

Merged
merged 1 commit into from
Dec 10, 2019

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Dec 10, 2019

Motivation

Currently, automatic switching to a dark theme does not work if a dark theme is selected in the system (because even when there is no saved theme in local storage, we set (override init value with correct theme) the theme to null value which is cast to an empty string (i.e. light theme)). This PR solves this bug.

} else if (supportsColorSchemeQuery && mql.matches) {
setDataThemeAttribute('dark');

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

  1. Select a dark theme in your OS
  2. Go to the docs site (after clearing localstorage), a dark theme should be enabled.

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Dec 10, 2019
@docusaurus-bot
Copy link
Contributor

Deploy preview for docusaurus-preview ready!

Built with commit ba17d3e

https://deploy-preview-2113--docusaurus-preview.netlify.com

@docusaurus-bot
Copy link
Contributor

Deploy preview for docusaurus-2 ready!

Built with commit ba17d3e

https://deploy-preview-2113--docusaurus-2.netlify.com

Copy link
Contributor

@endiliey endiliey left a comment

Choose a reason for hiding this comment

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

Shouldnt it be

let localStorageTheme = null;
try

catch

if locaStorageTheme !== null,
setTheme

@lex111
Copy link
Contributor Author

lex111 commented Dec 10, 2019

I don't think so, the current code is simple and working.

Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

Thanks, I noticed this bug and saw that the page turned dark then turned back to light so I suspected it was this took overriding it after the inline script set it. Great job @lex111!

This is the correct fix @endiliey

@yangshun yangshun merged commit bbc1d2c into facebook:master Dec 10, 2019
@endiliey
Copy link
Contributor

Yeah this is correct because getItem will always return null if key does not exist (local storage cleared). Always thought it was undefined.

Although i believe this below still achieve the same.

let localStorageTheme = null;
try {
localStorageTheme = localStorage.getItem(‘theme’)
}
catch {}
if (localStorageTheme !== null) {
setTheme(localStorageTheme)
}

@yangshun
Copy link
Contributor

yangshun commented Dec 11, 2019

@endiliey where are you suggesting to add that?

I don't think the above code will work if you meant to put it in the fouc script.

@endiliey
Copy link
Contributor

endiliey commented Dec 11, 2019

^ in the same code of this PR

Its just the same. But more code

Just forget it lol. 🤣

@endiliey endiliey added the pr: bug fix This PR fixes a bug in a past release. label Dec 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants