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

Add iOS style dark/light theme toggle #118

Merged
merged 10 commits into from
Jan 19, 2019
Merged

Add iOS style dark/light theme toggle #118

merged 10 commits into from
Jan 19, 2019

Conversation

gc
Copy link

@gc gc commented Jan 18, 2019

This is a cleaned up version of #44 with the requested iOS style toggle.

Adds a toggle which allows users to use a dark theme (persists in local storage), for light theme users there should be virtually no change apart from the addition of the toggle.

Live Demo: https://frosty-kirch-4a3ba9.netlify.com/

@gc gc mentioned this pull request Jan 18, 2019
@@ -4,7 +4,7 @@ import React from 'react'
import 'typeface-montserrat'
import 'typeface-merriweather'

import profilePic from './profile-pic.jpg'
import profilePic from '../assets/profile-pic.png'
Copy link
Owner

Choose a reason for hiding this comment

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

Why change to png?

Copy link
Author

Choose a reason for hiding this comment

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

here is a comparison between them:
photoshop_r6fwkntirx

mainly so that the background doesnt show as white in the dark theme. its also higher quality and uses CSS to make it rounded instead of the actual image itself being round.

{({theme, toggleTheme }) => (
<div style={{
color: theme.primary.text.normal,
background: theme.primary.background,
Copy link
Owner

Choose a reason for hiding this comment

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

Let's add transition: 'color 0.2s ease-out, background 0.2s ease-out'

}

.react-toggle-track {
background-color: #b3e5fc;
Copy link
Owner

Choose a reason for hiding this comment

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

I'd like to make this black.

<div style={{display: 'flex', justifyContent: 'space-between', alignItems: 'baseline'}}>
{this.renderHeader(theme)}
<Toggle
icons={{checked: <img src={moon}/>, unchecked: <img src={sun}/>}}
Copy link
Owner

Choose a reason for hiding this comment

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

Let's add alts to imgs.

primary: {
background: '#000000',
text: {
normal: 'rgba(255, 255, 255, 0.8)',
Copy link
Owner

Choose a reason for hiding this comment

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

0.9

@gc
Copy link
Author

gc commented Jan 18, 2019

requested changes have been made

https://frosty-kirch-4a3ba9.netlify.com/

@j-f1
Copy link

j-f1 commented Jan 18, 2019

Swiping the switch on iOS doesn’t work properly :/

@gaearon
Copy link
Owner

gaearon commented Jan 18, 2019

Let's fix iOS (maybe just fork the toggle and customize it in our own code?)

Also first render animates if we "remember" the dark mode from storage. Can first render be sync with correct mode so we don't transition?

@gaearon
Copy link
Owner

gaearon commented Jan 18, 2019

Also there's still blue hover — maybe let's make it something like #222?

@gaearon
Copy link
Owner

gaearon commented Jan 19, 2019

react-toggle seems pretty buggy :-(. I looked into the code but there's a lot of mutation going on and I'm not confident I can understand that logic and fix it. aaronshaf/react-toggle#133

@j-f1
Copy link

j-f1 commented Jan 19, 2019

Have you considered using Netlify @gaearon? It would be perfect for PRs like this since it automatically builds a preview for each PR.

@gaearon
Copy link
Owner

gaearon commented Jan 19, 2019

Maybe. We'll see.

@gaearon gaearon merged commit 12eb10c into gaearon:master Jan 19, 2019
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.

3 participants