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.

Sign up for GitHub

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

Jump to bottom

Add auto skin which honors the prefers-color-scheme media query #634

Merged
merged 14 commits into from
Sep 4, 2022
Merged

Add auto skin which honors the prefers-color-scheme media query #634

merged 14 commits into from
Sep 4, 2022

Conversation

sandervoerman
Copy link
Contributor

@sandervoerman sandervoerman commented Jan 9, 2022 •
edited
Loading

This is a follow-up to #594, and adds the following changes:

  • the auto skin renders in light or dark mode depending on the prefers-color-scheme media query;
  • code has been refactored so that auto and classic share the same color definitions for light mode, and auto and dark for dark mode.

Screenshots:

auto skin, on Windows 10 in light mode:

jlight

auto skin, on Windows 10 in dark mode:
jdark

@ashmaroli
Copy link
Member

Couple of comments:

  • The name classic was selected to denote legacy appearance (Minima 2 n older). Having it switch to a dark mode doesn't sit nicely with me. How about --- auto, classic, dark..?
  • The table details for the new variants include duplicate info. IMO, you could move that to an anchored section below the table and link to the section in the description cells.

@sandervoerman
Copy link
Contributor Author

sandervoerman commented Jan 9, 2022 •
edited
Loading

The name classic was selected to denote legacy appearance (Minima 2 n older). Having it switch to a dark mode doesn't sit nicely with me. How about --- auto, classic, dark..?

Sure! What would become the default skin, then? auto or classic?

@sandervoerman
Rename classic skins
1ae8c89 
- 'classic' is always light
- 'auto' uses the media query
@ashmaroli
Copy link
Member

What would become the default skin, then? auto or classic?

classic for now since it is probably what consumers of this theme via jekyll-remote-theme plugin expects. Lets not shock them right away.

Once we come up with a way to demo the skins on every OS / browser, we may / can switch to auto being the default, or may not, at all. However, if we decide to cut a new release before that, it'll remain classic.
IMO, this shouldn't be a one-person decision.

@ashmaroli
Copy link
Member

Um.. To reduce duplication or conflict from #594 getting merged before this, lets not make changes to the README on this branch just yet.. Let the document be finalized there first.

@sandervoerman
Copy link
Contributor Author

ok sorry! 😅

@sandervoerman sandervoerman changed the title Allow classic skin to honor prefers-color-scheme media query Add auto skin which honors the prefers-color-scheme media query Jan 9, 2022
@ashmaroli ashmaroli changed the title Add auto skin which honors the prefers-color-scheme media query Add auto skin which honors the prefers-color-scheme media query Jan 9, 2022
@ashmaroli ashmaroli marked this pull request as draft January 9, 2022 18:26
This was referenced Feb 24, 2022
Closed
Closed
@DoodlesEpic
Copy link

Closes #660
Any reason why this hasn't gotten any updates?

@sandervoerman
Copy link
Contributor Author

Any reason why this hasn't gotten any updates?

I think this PR is waiting for #594 which in turn is waiting for review...

@mattr-
Copy link
Member

mattr- commented Jul 13, 2022

#594 has been merged, so this PR can be updated against the lateest master and be moved out of draft mode when it's ready.

@sandervoerman sandervoerman marked this pull request as ready for review July 26, 2022 07:19
@sandervoerman
Copy link
Contributor Author

@mattr- Ready now, let me know if you need anything else.

@mattr-
Copy link
Member

mattr- commented Sep 4, 2022

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit d54be44 into jekyll:master Sep 4, 2022
jekyllbot added a commit that referenced this pull request Sep 4, 2022
@jekyllbot
Update history to reflect merge of #634 [ci skip]
7f4b79d
@jekyll jekyll locked and limited conversation to collaborators Sep 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mattr- mattr- mattr- approved these changes

Assignees
No one assigned
Labels
enhancement frozen-due-to-age
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sandervoerman @ashmaroli @DoodlesEpic @mattr- @jekyllbot