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 Dark Mode #115

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

Add Dark Mode #115

wants to merge 4 commits into from

Conversation

vishwa35
Copy link

@vishwa35 vishwa35 commented Aug 9, 2020

  • adds dark mode using a sass mixin and a short script
  • renamed all colors to avoid overlap with named values (black/white/grey) and make their purposes clear
    • added a couple colors for links, to avoid defaulting to -webkit-link (which doesn't work for dark mode)
  • replaced svgs for social icons with font awesome icons
  • added layouts/_default/_markup/render-link.html to open all links in new tabs (except internal ones)

@@ -0,0 +1 @@
<a href="{{ .Destination | safeURL }}"{{ with .Title}} title="{{ . }}"{{ end }}{{ if strings.HasPrefix .Destination "http" }} target="_blank"{{ end }}>{{ .Text }}</a>
Copy link
Author

Choose a reason for hiding this comment

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

open links in new tab

Comment on lines +26 to +48
lightTheme: (
text: #111,
low-neutral: #f7f7f7,
neutral-table-border: #eeeeee,
neutral: #9b9b9b,
high-neutral: #717171,
bg: #fff,
accent: #9013FE,
accent-text: #580E99,
accent-bg: #A56AD9,
),
darkTheme: (
text: #fff,
low-neutral: #717171,
neutral-table-border: #eeeeee,
neutral: #9b9b9b,
high-neutral: #f7f7f7,
bg: #111,
accent: #9013FE,
accent-text: #A56AD9,
accent-bg: #580E99,
),
);
Copy link
Author

Choose a reason for hiding this comment

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

renamed colors; for the neutrals (formerly light/darkGrey) high means closer to color of text and low mean closer to background (bg)

@WickedBrat
Copy link

WickedBrat commented Sep 29, 2020

Much needed feature. Thanks @vishwa35 🥳
A few knits though:

  • You don't need to delete the social icons entirely, changing the stroke="currentColor" to stroke="white" would give you a white version of the current icons. Refer wickedbrat.com
  • The theme, as is picked from localstorage takes a fraction of sec to actually load..

@jakewies
Copy link
Owner

@vishwa35 I apologize for the late response, real life has been keeping me pretty busy these days. I am going to take a look at the PR now but I also want to note that a few recent PRs as of this morning have caused some conflicts in your branch. Just wanted to ping you so you're aware.

Thanks for your effort on this!

@nasirhemed
Copy link

Any updates on this ? Would love to use dark mode for this theme

Comment on lines +10 to +12
<li>
<i class="fas fa-moon" id="dark-mode-toggle"></i>
</li>
Copy link
Owner

Choose a reason for hiding this comment

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

Can we get this item to have cursor: pointer so that it feels clickable?

Comment on lines +5 to +9
if (window.matchMedia('(prefers-color-scheme: dark)').matches) {
setTheme(localStorage.getItem("dark-mode-storage") || "dark");
} else {
setTheme(localStorage.getItem("dark-mode-storage") || "light");
}
Copy link
Owner

Choose a reason for hiding this comment

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

Great stuff! Prefer system is a great design decision.

@jakewies
Copy link
Owner

jakewies commented Jan 23, 2021

Much needed feature. Thanks @vishwa35 🥳
A few knits though:

  • You don't need to delete the social icons entirely, changing the stroke="currentColor" to stroke="white" would give you a white version of the current icons. Refer wickedbrat.com
  • The theme, as is picked from localstorage takes a fraction of sec to actually load..

I second @WickedBrat here. I don't find it terribly important that we continue to support these inline svg icons that have been included in the project since its inception, and having the ability to use Font Awesome is always a nice win, but I think this feature might be better implemented in a separate PR where we can discuss it further. I'd like to keep this one as streamlined as possible, i.e., dark mode. Because it's a neat feature that clearly many people are looking for.

Now, as far as the dark mode implementation itself, it looks aces. However, I am also noticing this split-second flash of light mode before dark mode takes effect. Is there are a way to avoid this @vishwa35 ? It isn't a pleasing experience for the end-user.

All-in-all this was clearly a ton of effort on your part and I'm glad you've decided to include your PR to this project. I'd love to get it merged ASAP. But we should iron out these few things:

TODO

  • Continue to support inline svg for now using the suggestion @WickedBrat has mentioned in his comment.
  • Find a way to prevent this flash of light mode if dark mode is enabled
  • Add cursor: pointer to the toggle in the menu
  • resolve conflicts

@lzlrd
Copy link

lzlrd commented Feb 18, 2021

I've sent a different PR utilising the prefers-color-scheme property at #159. I think this is the way forward, as modern clients can now automatically switch between dark and light mode based on their system settings.

EDIT: If a toggle is still wanted/required, you could alternatively replace the prefers-color-scheme checks with a custom property that defaults to the system property but modified via JS.

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