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 lightmode based on system preference #64

Closed
wants to merge 1 commit into from

Conversation

david-infi
Copy link
Contributor

This edit adds a light mode design to the Infi Way. It is used based on the system settings of the user. Most people probably have light mode on by default, but maybe we want to default to dark mode unless specifically set. This is currently not supported so might need to change the code to support this behaviour.

@netlify
Copy link

netlify bot commented Jun 9, 2023

Deploy Preview for the-infi-way ready!

Name Link
🔨 Latest commit b28159a
🔍 Latest deploy log https://app.netlify.com/sites/the-infi-way/deploys/6482f6948a56ec00089f594e
😎 Deploy Preview https://deploy-preview-64--the-infi-way.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@david-infi david-infi marked this pull request as draft June 9, 2023 09:54
@david-infi david-infi marked this pull request as ready for review June 9, 2023 10:02
@jeroenheijmans jeroenheijmans self-requested a review June 12, 2023 06:53
@david-infi david-infi marked this pull request as draft June 12, 2023 08:52
@david-infi
Copy link
Contributor Author

I want to edit the background SVG so that it can be used for both the dark mode and the light mode version (with some dynamic CSS). Right now there are two SVGs; one for the dark mode and one for the light mode.

@david-infi david-infi marked this pull request as ready for review June 13, 2023 10:12
@david-infi
Copy link
Contributor Author

I was unable to find a clean way to change the fill / opacity of the background SVG 😢 I am leaving it as is, with two different SVGs.

Copy link
Member

@jeroenheijmans jeroenheijmans left a comment

Choose a reason for hiding this comment

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

Code looks good to me, cheers!

You mention you had to duplicate the SVG, I had a quick look if I could still come up with something based off mask-image and top level fill="..." in the svg but couldn't get it to work either; I reckon two SVG's is just fine and pragmatic.

I think it's great we'll be offering a light mode version but agree that the combination of folks usually having a light themed OS and our website looking far better in dark mode is not great. Tailwindcss.com seems to have had the same consideration, and they default to dark mode with a toggle. The downside of a toggle would be that we either need to include JS to add it (which we try to avoid as much as possible for a "document" style website as long as we can manage it), and/or would need to become part of the URL or localStorage or something. But I guess it's worth it, and could be added to the <script> block in the template.html file? If we do, try to keep it as lightweight as possible. (PS. Perhaps @LucaScorpion has some thoughts on this as well? He helped out with the original code for Infi Way.)

@LucaScorpion
Copy link
Contributor

Very nice work! And I have to say, despite generally not being much of a light-mode person myself, I think it looks quite good. So no comments there from me! 🙌

I was unable to find a clean way to change the fill / opacity of the background SVG cry I am leaving it as is, with two different SVGs.

Yeahhh, I remember also looking into that a bit, but I also couldn't find a clean way of doing it. But 2 SVGs is fine I think, especially since I don't expect them to change anytime soon.

I do agree with Jeroen that just going by system prefs and not having a toggle on the site feels... less than ideal. I think a toggle for that could be done fairly easily with a simple toggle/slider and a couple of lines of JS in the script tag. I'm personally quite fond of the way https://react.dev/ does it, by defaulting to your system pref and just having a button to toggle the theme. This is a bit simpler than a menu like on https://tailwindcss.com/. Imo the "system" option is kinda nice, but generally unnecessary.

Side note: I'm also a fan of these kinds of toggles, cause they look real fancy 🤩 Especially if they're animated.

image

On the other hand, we could also defer implementing something like that to a separate issue/PR? I don't have a super strong opinion about that, and arguably this is already nicer than everyone being stuck on dark mode.

@jeroenheijmans
Copy link
Member

I also prefer the toggle style buttons. A little bit of JS and localStorage seems a pragmatic approach. Personally though I would default to dark and not the user settings, just because I think dark mode is so much nice to look at 😅

PS. Those buttons you copied here @LucaScorpion do we have appropriate (copy)rights to include them with current license, and choose a different license for them later on? Otherwise I could hop into Figma and try to create something too.

@LucaScorpion
Copy link
Contributor

@jeroenheijmans

do we have appropriate (copy)rights to include them with current license

Ha no, they're just a quick example I duckduckgo'd 😅 More to illustrate the idea of the slider with sun/moon icon in it :)

Otherwise I could hop into Figma and try to create something too.

That would be great!

@david-infi
Copy link
Contributor Author

I have been working on this. Attached is a video of a toggle I have been working on. It's still a work in progress, because I would like to improve the stars and clouds.

firefox_ac70rQFz4f.mp4

@jeroenheijmans
Copy link
Member

@david-infi Nice! Thanks for picking that up I slacked a little.

Is it SVG by any chance? We've not been super clear about it but we try to keep the Infi Way documents super self-contained, perhaps even go back to single-html-file setup some day ... so a visual that can be easily inlined would be nice.

@drikusroor drikusroor linked an issue Aug 15, 2023 that may be closed by this pull request
@LucaScorpion
Copy link
Contributor

Closing this, as it has been completed in #81

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

Successfully merging this pull request may close these issues.

Light mode based on system preferences
3 participants