-
Notifications
You must be signed in to change notification settings - Fork 323
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
ENH: Allow CSS variables to manipulate theme #190
ENH: Allow CSS variables to manipulate theme #190
Conversation
Internet Explorer has not been checked. I do agree with @choldgraf remark in #138 and don't think the target audience relies heavily on IE support. |
Also import more complete family for diversity
d299180
to
c239050
Compare
Cool! Will take a closer look later, but some very quick things I noticed:
And fine to lave out breakpoint configuration for this PR, there is already enough other content to focus on ;) (we could always add later a sphinx config variable for this, if it turns out difficult with css variables) |
This complements $grid-breakpoints instead of replacing it
Thanks for noticing @jorisvandenbossche. I replaced bootstraps |
43b9b15
to
aa92528
Compare
nice! so how would a user go about updating one of these variables for their builds? Just define them in a And is it possible to do this in a downstream theme (like sphinx-book-theme) that builds its CSS separately from pydata sphinx theme? If not, is there a technical path to make that possible? I'm happy to try building in something extra to the SBT if it's possible. |
Indeed. I was just testing this with contextily, and I can redefine the primary color variable instead of explicitly styling the h1 and h2 color: jorisvandenbossche/contextily@85c8f87 For the other aspects I am overriding there (eg the highlight color in the navigation sidebar), it seems we are still missing some variables
Do you mean that you are builing CSS from SASS as well, so you need to override the variables in SASS instead of CSS ? |
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.
Thanks a lot @hoetmaaiers
From how I was using it in contextily (the colors I was overriding there: https://github.com/geopandas/contextily/blob/master/docs/_static/css/custom.css), I think additional variables for those aspects would be welcome:
- The color to highlight the "active" navigation items (in the left sidebar, but also in the right TOC)
- The color for the
code
element (of course, this one is also easy to override with css since it is a single element to override, which is not the case for the nav highlight colors)
I assume that @choldgraf will also be able to say which aspects he is overwriting in sphinx-book-theme (https://github.com/executablebooks/sphinx-book-theme/blob/master/sphinx_book_theme/static/sphinx-book-theme.scss), for which it would be useful to have a variable (so those things that would specifically reduce their scss, or would make it easier to customize)
--fs-h2: 32px; | ||
--fs-h3: 26px; | ||
--fs-h4: 21px; | ||
--fs-h5: 28px; |
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.
this seems a typo? (28 doesn't really fit between 21 and 18 ;))
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.
yes indeed!
--fs-base: 15px; /* base font size - applied at body / html level */ | ||
|
||
/* heading font sizes */ | ||
--fs-h1: 36px; |
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.
Should we make those relative (em
) sizes?
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.
we could make those relative, but since we allow this to be defined in a 'setting', defining via em
is harder to think about. Since 1em == --fs-base
. Since mostly no inheritance happend on heading level, defining in pixels or ems is little difference I think.
/* admonition colors */ | ||
--color-admonition-base: var(--blue); | ||
--color-admonition-attention: var(--orange); | ||
--color-admonition-attention-light: #ffedcc; |
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 am not sure we need to necessarily add variables for all of those admonition types (we can always add later if there is demand for it). Maybe we could keep it on the 4 colors we use now.
What I was originally thinking for those admonitions is to reuse the bootstrap success
, danger
, warning
, info
variables.
The problem though is that we also need the "-light" version, and that doesn't seem fully straightforward. Eg I found https://codyhouse.co/blog/post/how-to-combine-sass-color-functions-and-css-variables, that has a way to allow something like lightness(var(--color-primary), 1.2)
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.
Limit the admonition types is fine for me, semantics in css are not that important I think.
Regarding the *-light
variant, this possible for sure, it could be looked upon as a clear theme choice.
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 diggel into the sass color functions combined with css variables. All solutions available start from defining the colors either based in RGB (e.g. red: 255, 0, 0
) or HSL (e.g red: 0%, 100%, 50%
.
With RBG, it is possible to lighten a color by adding an alpha channel. Darkening the color is harder I think.
HSL provides more flexibility.
The main disadvantage is the theme colors MUST be defined in either RGB, HSL and never Hex. Somehow this feels like a downside for the theme user who quickly wants to define his/her theme colors on top?
@jorisvandenbossche @choldgraf do you have thoughts on this?
Another example of a doc site with some colors customized: http://docs.momepy.org/en/latest/index.html (and their custom css: https://github.com/martinfleis/momepy/blob/master/docs/_static/custom.css) |
@jorisvandenbossche I'm a little bit confused now haha, in your CSS link you just showed "regular" CSS overrides, no SCSS variables etc, no? Then I'm not sure why this isn't already possible - couldn't someone already over-ride the CSS manually anyway? @hoetmaaiers can you please document how to use this feature so that others can try it out? I find that writing docs first is a great way to reverse-engineer the functionality you want and it helps people test out the PR as well :-) |
Yes, I am doing it in CSS (in the end, that's what most end users want, they shouldn't need SCSS), but it's not a "regular" override. Note that in the commit I linked I removed a "regular" override and changed that with overriding a variable. |
oh interesting - I'm not familiar with this pattern of:
and so maybe I am not doing things properly (in which case I re-iterate my request above that we document how to do this haha) |
That's the difference between CSS variables and SCSS variables. SCSS variables are compiled away by sass, the CSS variables are kept in the output (you can use CSS variables in the SCSS files as well, and then those are kept in the output CSS, so that you can still override this as a user of the theme. SCSS variables can't be overriden by downstream users, unless they would start from our scss instead of css). For sure we need to document this, but so what I posted above was a first try to see if it actually works what we would want to document ;) But basically it is:
Is that somewhat clear? Now, the above is for the typical user that will use css to override elements. |
TIL there are CSS variables and they are different from SCSS variables :-) let me try it and see if it works. So it must be under |
Great explanation on the difference between css and sass variables, thank you 👏@jorisvandenbossche. The goal was indeed to allow a user of the theme to only configure the theme without installing and building all sass, js, webpack related items. With this first PR I was looking for some opinions about the way of working, those I received, thank you 😉. I'll process the feedback and get back with some updates. |
I am thinking more about adding extra variables or limiting them to a fixed and more semantic set of variables. To me the difference is about either configuring or theming:
Theming equals the theme making clear decisions about what type of color is used in what place. Configuration hands over all choices to the end implementation. Personally I lean towards theming, since the configuration idea can also be achieved by applying a custom.css stylesheet on top of the default css. What are you thoughts about this? |
I think I agree with your intuition, to me it maps on to different user personas as well: Most people are not CSS experts and don't want to bother with tweaking lots of things, for them, some degree of control is helpful but not total control because it's overwhelming. Theming is a good way to control this level of control. More advanced or opinionated users will be both more knowledgeable and more motivated to do the manual tweaking themselves, and for this group it should be possible if not necessarily super easy to control each of the things they want (e.g. by writing their own |
It doesn't have to be one or the other. Both ways could be used together in the most customised version of the theme I think 😉. Good documentation will be key for this. I wait for other opinions ons this matter and will then reconsider the variables list. Your suggestion @choldgraf for starting documentation first is a good idea. |
I think we can go for some middle ground here? I suppose we could define some general theme colors (and actually using the bootstrap variables for this might also be possible? like the primary and secondary color variables). And then a few more detailed variables can be defined in terms of those theme variables? Now, for the concrete implementation in this PR, I would start to define those variables that seem needed in practice (based on the few examples we have that are already customizing this theme, so we can see to cover their needs). And then based on that, we can maybe have a more concrete discussion on configuring vs theming? |
I see this effort has slowed some, but: yes! Please expose as much configurability as possible in a way that a non-nodejs user can theme to their heart's content with documented web standards in a single file vs having to rebuild the assets with nodejs and whatever SCSS happens to support. Things this approach can offer out-of-the-box without having to re-load (much less rebuild):
|
Thanks for the feedback @bollwyvl ! That would indeed be good to enable such features like the one you mention (that let's me think of the nigh mode of the docker docs: https://docs.docker.com/develop/dev-best-practices/, the switch is in the right sidebar) |
Yeah, CSS-only can go pretty far: The genius hack is having a hidden However, there's no HTML5/CSS way I know of to persist this, so it would be reset on the next page viewed: for that, it would take a wee bit o' javascript to cram some stuff into localStorage/cookie... turns out, that's precisely what docker is doing. They aren't using variables, however, and certain things (like embedded widgets) are already mounting to |
I am planning to merge this: I think it is more than good enough for a start, and can then refine in follow-up PRs where needed (when actually using it in downstream docs) |
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.
Cool stuff! 🎉
/***************************************************************************** | ||
* Color | ||
* | ||
* Colors are defined in rgb string way, "red, green, blue" |
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 long as the custom CSS is added after the theme's CSS, it'll work. :)
* | ||
* Colors are defined in rgb string way, "red, green, blue" | ||
**/ | ||
--color-primary: 19, 6, 84; |
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.
OOOOO. It's soooo hacky, that I like it. XD
OK, let's see how this goes. Feedback on what is exactly exposed as variables is still very welcome! Example PR of how this simplifies customizing the navigation colors: geopandas/geopandas#1666 Thanks @hoetmaaiers for getting this started! |
wow, very exciting to see this one merged in! |
Great to see this one make it into the master branch. Will be made good use of future features! |
This is a first attempt at manipulating the base theme via variables. When a new variable is added, it is twice as hard to get rid of it, therefore I wanted some initial thoughts on the current approach in variable choices.
Some colors, sizes, ... are theme specific and not configurable. I think these possibilities should be discussed when the need arises?
Thing to note, breakpoint configuration is not included. This is hard to combine with the Bootstrap responsive utility classes. In other words: breakpoints can't be defined via native css variables, it does work via sass variables. https://stackoverflow.com/questions/40722882/css-native-variables-not-working-in-media-queries.
Small improvement (sorry for enlarging the PR this way), the font families included now include more variating regarding weight and italic possibilities. Looks immediately better IMHO (but could be finetunen in a later PR).