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

Dark Theme Tweaks #1062

Merged
merged 8 commits into from
Jan 16, 2019
Merged

Dark Theme Tweaks #1062

merged 8 commits into from
Jan 16, 2019

Conversation

roundhill
Copy link
Contributor

I'd like to propose we change the dark them to match up with the recent changes we've made to the iOS and macOS apps. It essentially desaturates the 'blueness' of the dark theme, and increases the contrast by making the text a bit lighter and the background color darker. The border color is also way more subtle than before.

Before:
screen shot 2018-11-30 at 3 05 53 pm

After:
screen shot 2018-11-30 at 3 09 27 pm

To Test

  • Enable the dark theme, and explore the app to ensure everything looks good!
  • Also ensure that the light theme still appears correctly (It uses the $gray* variables which were slightly tweaked in this PR)

@roundhill roundhill added the enhancement Improve existing functionality label Nov 30, 2018
@roundhill roundhill requested a review from mirka November 30, 2018 23:12
Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

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

Thanks, this new Dark color scheme now meets AA 🎉

Just one thing — we should make sure that the gray (used as text color for excerpts & dialogs) in the Light theme isn't less accessible as a side effect.

@@ -6,10 +6,10 @@ $blue-light: lighten($blue, 20%);
$blue-active-highlight: lighten($blue, 10%);

// Grays
$gray: #899199;
$gray: #999999;
Copy link
Member

Choose a reason for hiding this comment

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

#899199 against a white background wasn't AA-compliant to begin with, so we can't make it even lighter in the Light theme.

I did a quick contrast check the other day, and to meet AA it looks like we won't be able to share the same blue and gray values across both Light/Dark themes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah! I've updated it to #808080, which is AA-compliant for the white theme. The same color in the dark theme should now adjust it to be a bit lighter where it is used to #999999 (#808080 isn't AA-compliant against the #1E1E1E bg)

I also snuck in a fix for the markdown preview with inline code syntax, it was changing the font size and looked a bit odd.

Copy link
Member

Choose a reason for hiding this comment

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

Dark theme — The grays used for .dialog-tabs-button and .settings-group p text need to be lightened. (Currently at contrast ratio 2.95)

Light theme — I'm not sure where you got #808080 as AA-compliant for normal text against #FFF, since it is at contrast ratio 3.95. But I'm ok with this value for this PR, since it isn't worse than the current state and we need to make other tweaks to the Light theme colors anyway 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I must be mistaken about it being compliant. But it is what Apple uses for secondary text:

screen shot 2018-12-03 at 8 58 42 am

Copy link
Member

Choose a reason for hiding this comment

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

Interesting! It guess it's ok for Cocoa apps because macOS has an "Increase contrast" option in Accessibility settings...?

@mirka mirka modified the milestone: 1.3.2 Dec 3, 2018
Adding a font size fix for `inline code` in markdown.
@roundhill
Copy link
Contributor Author

@mirka any more thoughts on this?

@mirka
Copy link
Member

mirka commented Jan 9, 2019

Dark theme — The grays used for .dialog-tabs-button and .settings-group p text need to be lightened. (Currently at contrast ratio 2.95)

I think we can merge once this is addressed ☝️

@roundhill
Copy link
Contributor Author

Dark theme — The grays used for .dialog-tabs-button and .settings-group p text need to be lightened. (Currently at contrast ratio 2.95)

OK I've added some fixes for those in ea7d690

screen shot 2019-01-14 at 12 08 52 pm

Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

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

Thanks! 🎨

scss/theme.scss Outdated Show resolved Hide resolved
@roundhill roundhill merged commit 84f0540 into master Jan 16, 2019
@roundhill roundhill deleted the update/app-colors branch January 16, 2019 06:07
@randallagordon
Copy link

I finally upgraded to a version that includes this change—is it just me, or is this a bit...too contrastical? 🧐

I'm very much a 👍 for meeting AA ratios for folks that need it (as I probably will in a few years!), but I've had to switch back to light mode while I wait for other apps to add dark modes so I can drop my display brightness overall to compensate for how searing the white now is on this black. Any chance for getting a third theme option added? I'd be happy to work on a PR for it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants