Customization via CSS variables #282

zanderle · 2024-03-29T13:13:13Z

This PR makes the notifications a bit more reliant on the CSS variables. I tried doing the same for search as well, but focused mostly on colors. Making the full design rely on font-size is a bit more tricky for the search modal, because there's more involved than the "simple box" that are the notifications. We can always improve as we move further.

I also had to change the setting of the default CSS variable on flyout from :root to :host as the former didn't seem to work.

Closes #197

In order to make the notifications box customizable when it comes to font-size, we need to ensure other aspects (like paddings, line-heights, etc) are changed accordingly as well.

…an :root For some reason, defining the initial CSS variable on :root doesn't work. The easy workaround (which also makes it a bit more flexible) is to define it on :host instead.

humitos · 2024-04-08T10:25:19Z

Making the full design rely on font-size is a bit more tricky for the search modal, because there's more involved than the "simple box" that are the notifications

Should we have a specific variable for this? I don't know, maybe readthedocs-<addons>-size: 100% where all the size/spacing/etc are calculated based on that variable? Would a variable like this make this easier?

zanderle · 2024-04-08T10:39:03Z

Making the full design rely on font-size is a bit more tricky for the search modal, because there's more involved than the "simple box" that are the notifications

Should we have a specific variable for this? I don't know, maybe readthedocs-<addons>-size: 100% where all the size/spacing/etc are calculated based on that variable? Would a variable like this make this easier?

That's a good idea. But no, it's more a matter of going through every single design element in the search modal, determining how it should relate to the font-size (or some kind of a global variable) and making sure it works well with it. It's just a time-consuming process more than anything else.
If you think it's an important feature to have, I can still add that.

agjohnson

All of this seems good. It is indeed a shame there isn't a way to reference the root shadow dom element using an absolute unit like rem.

humitos · 2024-04-11T15:20:42Z

I will try to test this locally soon and merge after that.

humitos · 2024-04-16T10:41:08Z

@zanderle @agjohnson how are you testing this? I added the following CSS and I'm not seeing any change:

:host {
  --readthedocs-flyout-font-size: 0.2rem;
  --readthedocs-notification-font-size: 0.2rem;
  --readthedocs-search-font-size: 0.2rem;
}

agjohnson · 2024-04-16T16:45:31Z

Did you try setting those variables on <html> or something top level and outside the shadow DOM?

humitos · 2024-04-17T10:18:10Z

Using html seems to work:

html {
--readthedocs-flyout-font-size: 0.63rem;  /* Material for MkDocs */
}

@agjohnson Is this the correct way we want users to customize the sizes?

One thing I noticed here is the branch icon doesn't scale accordingly with this setting:

I found this because the size of this icon is bigger on Material for MkDocs than on Sphinx by default (same as the font), but changing the --readthedocs-flyout-font-size doesn't make the icon to scale accordingly. @zanderle Is it possible to consider this as well?

zanderle · 2024-04-17T11:28:52Z

Using html seems to work:
html {
--readthedocs-flyout-font-size: 0.63rem;  /* Material for MkDocs */
}
@agjohnson Is this the correct way we want users to customize the sizes?

You can also set it on body or on the web-component itself, and it should work the same.

One thing I noticed here is the branch icon doesn't scale accordingly with this setting:

I found this because the size of this icon is bigger on Material for MkDocs than on Sphinx by default (same as the font), but changing the --readthedocs-flyout-font-size doesn't make the icon to scale accordingly. @zanderle Is it possible to consider this as well?

Yes, I'll add that. Good catch.

humitos · 2024-04-17T12:11:30Z

@zanderle can you tell me what are the steps you are following to test this? I'm having issues with. I'm not sure why sometimes it works and sometimes it doesn't. I may not be following the correct steps here.

zanderle · 2024-04-17T14:18:04Z

@zanderle can you tell me what are the steps you are following to test this? I'm having issues with. I'm not sure why sometimes it works and sometimes it doesn't. I may not be following the correct steps here.

When I'm testing, I do it in developer tools, and create the variable on the element itself:

humitos · 2024-04-17T14:42:19Z

OK, yeah. Testing it that way it works for me as well (and the SVG icon scales properly 👍🏼 ). However, it's still not clear to me how users should define these variables. We talked about defining them under :host, :root, html and/or body. However, none of them work for me now. @zanderle do they work for you?

@agjohnson how do we want users to define these variables? Can you provide an example about this?

humitos · 2024-04-17T14:58:18Z

I tried doing the same for search as well, but focused mostly on colors. Making the full design rely on font-size is a bit more tricky for the search modal, because there's more involved than the "simple box" that are the notifications. We can always improve as we move further.

I opened #292 to track and work on this in a following PR 👍🏼

zanderle · 2024-04-17T18:15:50Z

OK, yeah. Testing it that way it works for me as well (and the SVG icon scales properly 👍🏼 ). However, it's still not clear to me how users should define these variables. We talked about defining them under :host, :root, html and/or body. However, none of them work for me now. @zanderle do they work for you?

Yeah, I'm noticing it now too. Not sure about the "sometimes it works, sometimes it doesn't", but it looks like the css variable defined in the web-component using :host { } takes precedence over anything defined in the parent DOM (even with !important), since that's the scope it's in. However if we don't define it using :host { } we can't provide the default value for the variable.
The way around this is that we just have to define it in a way that's more specific than :host. So in the parent DOM we could either have

/* Using the element */
readthedocs-flyout {
  --readthedocs-flyout-font-size: 1.2rem;
}

or if we defined the webcomponent to have a specific class, we could do:

/* Using the class */
.some-specific-class {
  --readthedocs-flyout-font-size: 1.2rem;
}

...

<readthedocs-flyout class="some-specific-class"></readthedocs-flyout>

If that's not acceptable, I'll have to do some more research to see if I'm missing something

agjohnson · 2024-04-17T20:25:03Z

So, all of these options should operate the same, they are all valid methods.

However, our variables are scoped per-addon (--readthedocs-flyout-font-size) so I think our guidance should be setting the variables at the top level. At the parent, the html or :root selector are most common.

@agjohnson how do we want users to define these variables? Can you provide an example about this?

There isn't really a standard to point at here. MDN docs show :root, you'll commonly see vars on html too though. :root is just html with higher specificity.

https://developer.mozilla.org/en-US/docs/Web/CSS/:root

However if we don't define it using :host { } we can't provide the default value for the variable.

There are two ways I see us getting around this:

First, we can use an internal variable name that doesn't conflict with the outer variable name. Inside the flyout CSS for example:

:host {
  --readthedocs-inner-flyout-font-size: var(--readthedocs-flyout-font-size, 0.8rem);
}

This seems easy and low friction, I didn't test this though and maybe I'm wrong here.

Maybe a larger wrench in our plans than we wanted is a different version of the above though.

At the parent level, users could do any of these:

:root {
  --readthedocs-font-size: 19px;
}

readthedocs-flyout {
  --readthedocs-font-size: 20px;
}

readthedocs-flyout.bigger {
  --readthedocs-flyout-font-size: 21px;
}

And our addons set the variables as:

:host {
  --readthedocs-flyout-font-size: var(--readthedocs-font-size, 0.8rem);
}

p {
  font-size: var(--readthedocs-flyout-font-size);
}

Giving us shared CSS variables at html/:root/whatever and individual addons variables at the addon element specificity. Or combine the two so that users aren't thrown off setting --readthedocs-flyout-font-size at :root.

zanderle · 2024-04-18T08:42:39Z

There are two ways I see us getting around this:

First, we can use an internal variable name that doesn't conflict with the outer variable name. Inside the flyout CSS for example:
:host {
  --readthedocs-inner-flyout-font-size: var(--readthedocs-flyout-font-size, 0.8rem);
}
This seems easy and low friction, I didn't test this though and maybe I'm wrong here.

Yeah, this one should be pretty easy to do - it would just be a matter of renaming the "inner" variables.

Maybe a larger wrench in our plans than we wanted is a different version of the above though.

At the parent level, users could do any of these:
:root {
  --readthedocs-font-size: 19px;
}

readthedocs-flyout {
  --readthedocs-font-size: 20px;
}

readthedocs-flyout.bigger {
  --readthedocs-flyout-font-size: 21px;
}
And our addons set the variables as:
:host {
  --readthedocs-flyout-font-size: var(--readthedocs-font-size, 0.8rem);
}

p {
  font-size: var(--readthedocs-flyout-font-size);
}
Giving us shared CSS variables at html/:root/whatever and individual addons variables at the addon element specificity. Or combine the two so that users aren't thrown off setting --readthedocs-flyout-font-size at :root.

Yeah, I think the combination would be a good approach too. I'll try putting this together to see if there are any hidden problems and then we can decide on the final "format".

Allow for a general --readthedocs-font-size, but make it easy to override the specific addon with --readthedocs-<addon>-font-size

zanderle · 2024-04-26T15:36:59Z

I wasn't sure about the naming, but take a look at the last commit in this PR. How do you feel about this approach? @humitos @agjohnson

humitos · 2024-05-14T15:16:10Z

I wasn't sure about the naming, but take a look at the last commit in this PR. How do you feel about this approach?

Looks good to me 👍🏼

agjohnson · 2024-05-21T15:59:06Z

It looks like --addons-* is the internal variable name that we manage, and --readthedocs-* is the external, user controlled variable name?

@humitos you are generally a proponent of keeping the --readthedocs-* prefix the same on all variables, does having the two competing prefixes seem okay to you?

Otherwise this approach looks great to me.

humitos · 2024-05-22T09:32:00Z

I'm not too worried about the internal name. We can always change it later if we find a better one. However, I want to keep --readthedocs-* prefix for all variables we are exposing to users.

zanderle added 3 commits March 28, 2024 18:36

Make the font-size control sizing of notifications

12688c5

In order to make the notifications box customizable when it comes to font-size, we need to ensure other aspects (like paddings, line-heights, etc) are changed accordingly as well.

Change the customizable CSS variable to be defined on :host rather th…

8b677b6

…an :root For some reason, defining the initial CSS variable on :root doesn't work. The easy workaround (which also makes it a bit more flexible) is to define it on :host instead.

Add more customizable css variables to the search addon

5e17681

zanderle requested review from humitos and a team as code owners March 29, 2024 13:13

zanderle requested a review from agjohnson March 29, 2024 13:13

agjohnson approved these changes Apr 10, 2024

View reviewed changes

Merge branch 'main' into zanderle/css-variables

31f80d7

Adjust the branch icon size based on css variable

bdb8dfe

humitos mentioned this pull request Apr 17, 2024

Search: scale modal based on --readthedocs-search-font-size #292

Open

Change the approach for CSS variables

904a761

Allow for a general --readthedocs-font-size, but make it easy to override the specific addon with --readthedocs-<addon>-font-size

humitos mentioned this pull request May 6, 2024

Support dark/light mode based on browser's preferences #301

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Customization via CSS variables #282

Customization via CSS variables #282

zanderle commented Mar 29, 2024

humitos commented Apr 8, 2024 •

edited

zanderle commented Apr 8, 2024

agjohnson left a comment

humitos commented Apr 11, 2024

humitos commented Apr 16, 2024

agjohnson commented Apr 16, 2024

humitos commented Apr 17, 2024 •

edited

zanderle commented Apr 17, 2024

humitos commented Apr 17, 2024

zanderle commented Apr 17, 2024

humitos commented Apr 17, 2024

humitos commented Apr 17, 2024

zanderle commented Apr 17, 2024

agjohnson commented Apr 17, 2024 •

edited

zanderle commented Apr 18, 2024

zanderle commented Apr 26, 2024

humitos commented May 14, 2024

agjohnson commented May 21, 2024

humitos commented May 22, 2024

Customization via CSS variables #282

Are you sure you want to change the base?

Customization via CSS variables #282

Conversation

zanderle commented Mar 29, 2024

humitos commented Apr 8, 2024 • edited

zanderle commented Apr 8, 2024

agjohnson left a comment

Choose a reason for hiding this comment

humitos commented Apr 11, 2024

humitos commented Apr 16, 2024

agjohnson commented Apr 16, 2024

humitos commented Apr 17, 2024 • edited

zanderle commented Apr 17, 2024

humitos commented Apr 17, 2024

zanderle commented Apr 17, 2024

humitos commented Apr 17, 2024

humitos commented Apr 17, 2024

zanderle commented Apr 17, 2024

agjohnson commented Apr 17, 2024 • edited

zanderle commented Apr 18, 2024

zanderle commented Apr 26, 2024

humitos commented May 14, 2024

agjohnson commented May 21, 2024

humitos commented May 22, 2024

humitos commented Apr 8, 2024 •

edited

humitos commented Apr 17, 2024 •

edited

agjohnson commented Apr 17, 2024 •

edited