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 adaptive skin: solarized #594

Merged
merged 8 commits into from
Jul 13, 2022
Merged

Conversation

sandervoerman
Copy link
Contributor

@sandervoerman sandervoerman commented Jul 5, 2021

Hi all,
This PR changes the solarized skin so that it uses the prefers-color-scheme media query to detect whether the visitor's operating system is using dark mode:

  • if it is, then the skin will look exactly like solarized-dark;
  • otherwise, i.e. if the user prefers light mode or if no preference could be detected, then the skin will look like solarized-light;
  • the new solarized-light skin behaves like the solarized skin before this PR.

Additional notes:

  • The solarized-light and solarized-dark skins produce the exact same CSS as the solarized and solarized-dark skins before this PR.
  • The CSS produced by the new solarized skin setting is a bit different however, because it makes use of CSS custom properties to select the appropriate colors depending on the media query (see comment on IE support below).

@mattr-
Copy link
Member

mattr- commented Dec 29, 2021

I think this would be awesome to have for the default skins as well, since there's now a light and a dark skin. Is this something you'd be able to do in a follow up PR?

Copy link
Member

@mattr- mattr- left a comment

Choose a reason for hiding this comment

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

What do you think about renaming solarized to solarized-light and then renaming solarized-auto to solarized so that the prefers-color-scheme query is used by default?

@sandervoerman
Copy link
Contributor Author

What do you think about renaming solarized to solarized-light and then renaming solarized-auto to solarized so that the prefers-color-scheme query is used by default?

I like making solarized the query-aware version and solarized-light and solarized-dark the fixed alternatives. However, I would implement it slightly differently, so that the bulk of the code is still in the main solarized.scss file. I will try to do that soon.

I think this would be awesome to have for the default skins as well, since there's now a light and a dark skin. Is this something you'd be able to do in a follow up PR?

Sounds like fun! Would that mean adding a light skin that behaves like the current classic skin, and changing the classic skin to become aware of the query, similar to what you're proposing for the solarized skins?

@mattr-
Copy link
Member

mattr- commented Dec 30, 2021

What do you think about renaming solarized to solarized-light and then renaming solarized-auto to solarized so that the prefers-color-scheme query is used by default?

I like making solarized the query-aware version and solarized-light and solarized-dark the fixed alternatives. However, I would implement it slightly differently, so that the bulk of the code is still in the main solarized.scss file. I will try to do that soon.

Awesome! Thanks!

I think this would be awesome to have for the default skins as well, since there's now a light and a dark skin. Is this something you'd be able to do in a follow up PR?

Sounds like fun! Would that mean adding a light skin that behaves like the current classic skin, and changing the classic skin to become aware of the query, similar to what you're proposing for the solarized skins?

Yes, exactly. Looking forward to seeing that when you're able to make some time for it. 🙂

Setting the skin to 'solarized' now determines light or dark
automatically. The always light and always dark skins are named
'solarized-light' and 'solarized-dark' respectively.
@sandervoerman
Copy link
Contributor Author

I made the changes to the skin names so that solarized is now the skin that uses the prefers-color-scheme.

@sandervoerman
Copy link
Contributor Author

There is another thing to consider before merging this PR: do you still need to support Internet Explorer?

This implementation makes use of CSS variables, which IE does not support. So if you'd now test this new solarized skin setting on IE, it would end up ignoring all CSS rules that reference CSS variables from the skin. Note that his only affects the new solarized skin setting. The solarized-light and solarized-dark skins should work fine on IE.

All other browsers have been supporting CSS variables for years, and IE is going end of life in five months, but it might still be an unexpected result for users.

Of course there are possible workarounds, but they would require some tinkering with the rest of the minima skin mechanism. Normally, when you want to use CSS variables and also support IE, you just use the standard CSS fallback mechanism, i.e.:

.foo {
  color: black;            /* fallback for Internet Explorer */
  color: var(--my-color);  /* supported by all other browsers */
}

But of course we cannot wrap all that into a single Sass variable that is being assigned to a color attribute only once. There are different ways to solve this: load a different stylesheet if IE is detected, or rewrite the theme to use Sass mixins that allow fallback rules. But if supporting IE is not a requirement then I think sticking to the current implementation would be the most elegant.

@ashmaroli
Copy link
Member

@sandervoerman Please update the PR title and opening comment as well.
Thank you.

@sandervoerman sandervoerman changed the title New skin: solarized-auto Allow solarized skin to honor prefers-color-scheme media query Jan 9, 2022
@ashmaroli
Copy link
Member

do you still need to support Internet Explorer?

My take on this is that IE support is no longer a priority.
The current set of changes is already non-backwards-compatible with the last released gem version. This is the perfect time to drop support for a browser that is not going to rise to modern needs, ever.

@ashmaroli ashmaroli changed the title Allow solarized skin to honor prefers-color-scheme media query Add adaptive skin: solarized Jan 9, 2022
@mattr-
Copy link
Member

mattr- commented Jul 13, 2022

@jekyllbot: merge +major

@jekyllbot jekyllbot merged commit c489493 into jekyll:master Jul 13, 2022
jekyllbot added a commit that referenced this pull request Jul 13, 2022
@rrevi
Copy link

rrevi commented Jul 15, 2022

Thank you @sandervoerman and @mattr- for making this happen!

Now that this is in master, how do we get this into our own Jekyll apps? will this be part of a specific version of minima? Just looking for some guidance on how to update my Jekyll minima theme to include these updates.

@mattr-
Copy link
Member

mattr- commented Jul 15, 2022

The best way is to use the jekyll-remote-theme plugin and configure it to use minima and i believe (not 100% sure) that it'll pull the latest from GitHub and you'll have it.

@rrevi
Copy link

rrevi commented Jul 15, 2022

The best way is to use the jekyll-remote-theme plugin and configure it to use minima and i believe (not 100% sure) that it'll pull the latest from GitHub and you'll have it.

Thanks @mattr-

I updated my _config.yml file to look like:

...
minima:
  skin: solarized
  social_links:
    github: rrevi
    linkedin: rafaelrevi

remote_theme: jekyll/minima

plugins:
 - jekyll-feed
 - jekyll-seo-tag
 - jekyll-remote-theme

and when I run bundle exec jekyll build --verbose, I see the following output:

   Generating... 
         Theme: jekyll/minima
  Theme source: /private/var/folders/xx/lwkqbsn51s77jmbkgxk6mrf00000gn/T/jekyll-remote-theme-20220715-43229-1lkctb1
  Remote Theme: Using theme jekyll/minima
  Remote Theme: Downloading https://codeload.github.com/jekyll/minima/zip/HEAD to /var/folders/xx/lwkqbsn51s77jmbkgxk6mrf00000gn/T/jekyll-remote-theme-20220715-43229-1o70tf3.zip
  Remote Theme: Unzipping /var/folders/xx/lwkqbsn51s77jmbkgxk6mrf00000gn/T/jekyll-remote-theme-20220715-43229-1o70tf3.zip to /private/var/folders/xx/lwkqbsn51s77jmbkgxk6mrf00000gn/T/jekyll-remote-theme-20220715-43229-1lkctb1
     Requiring: jekyll-feed
     Requiring: jekyll-seo-tag

Which leads to me believe all is fine. However, when I run jekyll serve, and bring up the site in a (or any) browser, and have macOS in dark mode (where all apps immediately go dark), it does not show in solarized dark as I would expect.

Do you or @sandervoerman have any thoughts what might be going on?

Thanks in advance

@rrevi
Copy link

rrevi commented Jul 15, 2022

The best way is to use the jekyll-remote-theme plugin and configure it to use minima and i believe (not 100% sure) that it'll pull the latest from GitHub and you'll have it.

Thanks @mattr-

I updated my _config.yml file to look like:

...
minima:
  skin: solarized
  social_links:
    github: rrevi
    linkedin: rafaelrevi

remote_theme: jekyll/minima

plugins:
 - jekyll-feed
 - jekyll-seo-tag
 - jekyll-remote-theme

and when I run bundle exec jekyll build --verbose, I see the following output:

   Generating... 
         Theme: jekyll/minima
  Theme source: /private/var/folders/xx/lwkqbsn51s77jmbkgxk6mrf00000gn/T/jekyll-remote-theme-20220715-43229-1lkctb1
  Remote Theme: Using theme jekyll/minima
  Remote Theme: Downloading https://codeload.github.com/jekyll/minima/zip/HEAD to /var/folders/xx/lwkqbsn51s77jmbkgxk6mrf00000gn/T/jekyll-remote-theme-20220715-43229-1o70tf3.zip
  Remote Theme: Unzipping /var/folders/xx/lwkqbsn51s77jmbkgxk6mrf00000gn/T/jekyll-remote-theme-20220715-43229-1o70tf3.zip to /private/var/folders/xx/lwkqbsn51s77jmbkgxk6mrf00000gn/T/jekyll-remote-theme-20220715-43229-1lkctb1
     Requiring: jekyll-feed
     Requiring: jekyll-seo-tag

Which leads to me believe all is fine. However, when I run jekyll serve, and bring up the site in a (or any) browser, and have macOS in dark mode (where all apps immediately go dark), it does not show in solarized dark as I would expect.

Do you or @sandervoerman have any thoughts what might be going on?

Thanks in advance

With the use of the jekyll-remote-theme plugin, you want to make sure to remove any skin directory/files in the _sass/minima path.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants