Skip to content

Conversation

@moshenskyDV
Copy link

Feature #6

Solution: Added JS function and listener to save and handle current state in localStorage. Added new scss file with night mode styles (wrapped into .night-mode).

Copy link
Contributor

@cassidyjames cassidyjames left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on! A couple of initial critiques:

  • How does this interact with a browser's native dark style? I think the JS is going to need to be aware of whether or not the site is already in a dark style due to the browser/OS requesting a dark style.

  • We should make sure to only create the toggle UI when JS is available, otherwise there's a non-functional action for users who don't enable JS.

  • I think I'd like to use a switch or other UI control instead of a link, but I understand that requires more CSS work to style that. That could come in a future PR.

  • Can we combine the existing dark styles with the new class to avoid duplication?

@moshenskyDV
Copy link
Author

I've done point 2 and point 4 (mixins).
About prefers-color-scheme - we should discuss, how to figure it out. Right now, if user prefers dark scheme - anyway dark style will be used. There is couple of options:

  1. Remove prefers-color-scheme from styles, detect it by JS and apply night-mode class. Disadvantages: when JS disabled - night mode won't be applied (we can figure it out by <noscript> actually, but it makes code more complicated. And one more disadvantage - we will have short "flashlight" on page load (before JS is load).
  2. Just remove button when night mode enabled (fastest way, but may be not user friendly). If user already configured his browser to dark mode - he want dark site I think (just as option).

About 3 - what element, on your opinion will be semantic correct? Maybe <button type="button></button>?

@cassidyjames cassidyjames requested a review from a team October 7, 2019 18:56
@lewisgoddard
Copy link
Member

lewisgoddard commented Oct 7, 2019

We should definitely not be setting the page style in JS when CSS is available. JS just needs to know what state the page is in to override if the option is changed, not set it initially.

Personally I would use a checkbox styled as a toggle switch.

@voldyman
Copy link
Collaborator

voldyman commented Oct 7, 2019

JS just needs to know what state the page is in to override if the option is changed, not set it initially.

window.matchMedia("(prefers-color-scheme: dark)").matches will tell if the user prefers dark theme, which can be the initial state.

@moshenskyDV
Copy link
Author

Ok, I've added mirror mixins to light mode. It works without JS and with JS styles should overlap correctly. Check it please.

@moshenskyDV
Copy link
Author

Notify me please, if all is ok (I'm going to do rebase with squash).

@cassidyjames
Copy link
Contributor

cassidyjames commented Oct 22, 2019

My initial thought is that there is a lot of styling that's been pulled out into the mixins that is going to be hard to keep track of. I think keeping the components' styling in their original file would be more maintainable.

@jeremypw
Copy link

@danrabbit Has conflicts and no recent activity. Does this need closing or converting to draft? I do not have the necessary rights.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants