-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
use css vars and @media queries to change colours depending on device theme #135
base: master
Are you sure you want to change the base?
Conversation
We must use css vars because you can change them depending on whether the device is using light or dark theme and there is documentation on media quirys at https://developer.mozilla.org/en-US/docs/Web/CSS/@media/prefers-color-scheme
Welcome! Congrats on your first pull request to the Cayman theme. If you haven't already, please be sure to check out the contributing guidelines. |
I've bin' tryin to make the web just a liddle dark over the past few days |
theme nearly ready |
I have a working branch in https://github.com/godalming123/cayman/tree/support-for-changing-theme-with-_config.yaml this will allow you to change color-scheme based on config.yaml option however the only problem is it doesn't deafualt to auto theme so if you do not set |
…th-_config.yaml Support for changing theme with config.yaml
Just added support for changing the theme with a config.yaml option that deafualts to auto in patch-1 branch! |
README.md
Outdated
@@ -40,6 +40,7 @@ Additionally, you may choose to set the following optional variables: | |||
```yml | |||
show_downloads: ["true" or "false" (unquoted) to indicate whether to provide a download URL] | |||
google_analytics: [Your Google Analytics tracking ID] | |||
color-scheme: ["dark", "light", "auto" or "auto-deafualt-dark" auto is deafault setting and changes theme based on device theme, auto-deafualt-dark is the same except if you device does not support changing theme based on device theme it will deafault to dark and the others are fairly explanetory.] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: “default” is the correct spelling, multiple places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I've fixed all the occurrences in my commit below. Sorry I'm not the best at spelling and the maintainers arent really keeping track of pull request so I have stopped checking this pull.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think defaulting to "auto" here when the original is "light" might also be an unexpected change for anyone using "latest" versions / not using a specific version of this template. Defaulting to "light" might be less risk of surprises for users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I'm aware the only way that new users can come across this is if they find this PR, in which case they will only use it if they want their site to have a dark theme. Sure, if the maintainers get back to me then new people could discover it without knowing about dark theme but I think for now better to save the confusion if it does not work for people who are trying to use this theme.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
this is not stale |
This is useful functionality! Just say the repo is unmaintained |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great effort! I like the dark theme option a lot. It works almost. The included style in default.html layout results in a Liquid Warning:
Liquid Warning: Liquid syntax error (line 19): Unexpected character { in "{{ "/assets/css/color-schemes/colors-ColorScheme.css?v=" | append: site.github.build_revision | relative_url | replace: "ColorScheme", {{ColorScheme}}" in /_layouts/default.html
And the gradient background image is lost in dark mode. I tried to resolve these issues, but have not been successful yet. If these issues can be resolved, I see no reason why it couldn't be merged.
@markblokpoel thank you for finding these issues, I will see if I ever get time to fix them but don't get your hopes up. I kinda gave up on this when I realised that it will not be used in production if it is not merged; the Cayman theme is already being pushed by the github pages GUI but has little users, how many of them will switch to this theme when it requires realising that their are more themes then the ones suggested by the github pages GUI and others likely do a better job for their needs. And I doubt it will be merged based on the number of garbage changes that haven't been closed. |
We must use css vars because you can change them depending on whether the device is using light or dark theme and there is documentation on media quirys at https://developer.mozilla.org/en-US/docs/Web/CSS/@media/prefers-color-scheme. to see what it looks like turn dark mode on for your device and visit https://godalming123.github.io/cayman/