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

Respect user preference for color scheme #1456

Merged

Conversation

abhishalya
Copy link
Contributor

Automatically switches to 'non-primary' theme if user prefers dark scheme.
If there is a theme specified in the local storage then that takes more preference over this.

Closes #1320

@mortenpi mortenpi added Format: HTML Related to the default HTML output Type: Enhancement labels Oct 29, 2020
@mortenpi mortenpi added this to the 0.26.0 milestone Oct 29, 2020
Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

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

Currently it looks like it always defaults to the dark theme now, even though

> window.matchMedia('(prefers-color-scheme: dark)')
MediaQueryList {media: "(prefers-color-scheme: dark)", matches: false, onchange: null}

So it feels like something is broken here.

assets/html/themeswap.js Outdated Show resolved Hide resolved
assets/html/themeswap.js Outdated Show resolved Hide resolved
assets/html/themeswap.js Outdated Show resolved Hide resolved
@abhishalya
Copy link
Contributor Author

Tried to address all above comments, also confirmed now that it works as it is supposed to.

Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

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

Awesome, seems to work nicely now! A few more things that popped up as I was playing around with this:

  1. There is the if(typeof(window.localStorage) === "undefined") return; at the beginning of set_theme_from_local_storage().

    This will mean that in browsers where window.localStorage is undefined, but that prefer the dark theme, the theme will not be changed. So I think this should now be removed and the code below updated to make sure we handle the case where there is no localStorage.

    I also discovered that window.localStorage can be null (e.g. if you disable localStorage). So I also think we should check with window.localStorage == null instead.

  2. The theme picker in the settings dialog currently still shows "documenter-light" if the dark theme is selected by the browser. So we should update that code as well.

assets/html/themeswap.js Outdated Show resolved Hide resolved
src/Writers/HTMLWriter.jl Outdated Show resolved Hide resolved
assets/html/themeswap.js Outdated Show resolved Hide resolved
assets/html/themeswap.js Show resolved Hide resolved
Copy link
Member

@mortenpi mortenpi left a comment

Choose a reason for hiding this comment

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

It looks like I can push to this branch myself. Could you also bring the branch up to date with master and cherry-pick this CHANELOG commit: 2751b43.

Other than this and the one code suggestion, I think this is good to go 🙂

assets/html/themeswap.js Outdated Show resolved Hide resolved
assets/html/themeswap.js Show resolved Hide resolved
@mortenpi
Copy link
Member

Could you merge master into this again? Or tick the "allow maintainers to push to this branch"? I can't merge this at the moment, because CHANGELOG.md conflicts.

@abhishalya
Copy link
Contributor Author

Or tick the "allow maintainers to push to this branch"?

Sorry, since this is created from the MLH fork, I wasn't able to do that. Although I've asked Cory to give you access to the MLH repos, so you should be able to push directly :)

I've rebased the branch with master for now.

@mortenpi
Copy link
Member

Thanks @abhishalya!

@mortenpi mortenpi merged commit 60f368c into JuliaDocs:master Nov 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Format: HTML Related to the default HTML output Type: Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Respect users' preference for light/dark mode
2 participants