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

Support For Mojave Dark Mode #68

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

chrisgrande
Copy link

Changed table view to use system colors to provide support for Mojave Dark Mode.
dark_mode

Copy link
Collaborator

@fulldecent fulldecent left a comment

Choose a reason for hiding this comment

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

Code-level review. Looks great.

@jakob
Copy link
Owner

jakob commented Dec 17, 2018

I'm not very happy with the color choices. Colors like unemphasizedSelectedTextBackgroundColor have a very specific function, and should not be used for unrelated things. Either use a custom color (use asset catalog to create custom color with light / dark mode), or use a color that is suitable from a semantic point of view (is there a system grid color?)

@chrisgrande
Copy link
Author

chrisgrande commented Dec 18, 2018

It's a good point. I had tested gridColor but it was very dark and wasn't being correctly color mixed with the Desktop picture - this now seems to be the case with unemphasizedSelectedTextBackgroundColor which does't get mixed in Dark Mode. Changes in 10.14.2 perhaps?

I have switched to gridColor (it seems to be working correctly now) for the grid lines and kept the unused table sections as alternatingContentBackgroundColor as it does the right thing in Dark Mode and adapts to the Desktop Picture.

@tyson90
Copy link

tyson90 commented Jul 9, 2020

Any plans to merge it?

@javinladish
Copy link

I'd love to have dark mode on my Table Tool app. Any plans on updating the app to support dark mode?

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.

5 participants