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

Solarized skins #404

Merged
merged 9 commits into from Nov 20, 2019
Merged

Solarized skins #404

merged 9 commits into from Nov 20, 2019

Conversation

sandervoerman
Copy link
Contributor

@sandervoerman sandervoerman commented Sep 12, 2019

Hi all,
I have created two skins, using the method proposed in #336. Both skins use the Solarized color scheme, one in light mode and the other in dark mode.

minima-solarized

screenshot

click here for minima-solarized theme preview

minima-solarized-dark

screenshot

click here for minima-solarized-dark theme preview

ashmaroli and others added 4 commits Jan 16, 2019
The 'solarized' skin was adapted from 'classic' using the Solarized
color scheme (see comments for reference and details).

The 'solarized-dark' skin is implemented as a helper file which
passes control to 'solarized' after setting $sol-is-dark.
DirtyF
DirtyF approved these changes Nov 15, 2019
@ashmaroli
Copy link
Member

@ashmaroli ashmaroli commented Nov 15, 2019

Since this uses commits from PR #336, that PR needs to be reviewed and merged first..

@sandervoerman
Copy link
Contributor Author

@sandervoerman sandervoerman commented Nov 15, 2019

Thanks @DirtyF, @ashmaroli!

Since this uses commits from PR #336, that PR needs to be reviewed and merged first..

I understand. I am happy to update this PR should further changes be made to PR #336 (other/different variables, etc).

@ashmaroli
Copy link
Member

@ashmaroli ashmaroli commented Nov 15, 2019

@sandervoerman #336 has been merged. You may update this branch.
Additionally, it'd be better if you could limit the whitespace for alignment in minima-solarized.scss to the current block. i.e. The skin color scheme values need not align with the values in section about Minima color scheme values

@sandervoerman
Copy link
Contributor Author

@sandervoerman sandervoerman commented Nov 16, 2019

Additionally, it'd be better if you could limit the whitespace for alignment in minima-solarized.scss to the current block. i.e. The skin color scheme values need not align with the values in section about Minima color scheme values

Done!

@ashmaroli
Copy link
Member

@ashmaroli ashmaroli commented Nov 18, 2019

@sandervoerman The header cells for a table are rendered with a lighter border in the solarized-dark skin. Is that intentional or a bug?

Screenshot_2019-11-18 This post demonstrates post content styles

@sandervoerman
Copy link
Contributor Author

@sandervoerman sandervoerman commented Nov 18, 2019

@sandervoerman The header cells for a table are rendered with a lighter border in the solarized-dark skin. Is that intentional or a bug?

It is not intentional, but I think the bug is in _base.scss: while it sets the table border color to $table-border-color, that is overruled for td elements by the td border color, which is set to $brand-color-light. The latter has the same value as the former in the classic skin, but not in the solarized skins.

The commit I just pushed would fix this (for all skins) by setting the td border property to $table-border-color as well (which I am guessing was your intention when defining the $table-border-color variable).

After this commit, the borders all have the same color in solarized-dark:

Screenshot-2019-11-18

@ashmaroli
Copy link
Member

@ashmaroli ashmaroli commented Nov 20, 2019

Thank you very much for working on this @sandervoerman

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 6d3a537 into jekyll:master Nov 20, 2019
6 checks passed
@sandervoerman sandervoerman deleted the minima-solarized branch Nov 21, 2019
@jekyll jekyll locked and limited conversation to collaborators Nov 20, 2020
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.

None yet

4 participants