More variable fixes #487
More variable fixes #487
Conversation
This moves the inner CSS rules from inside the shadow DOM to the parent DOM. This allows users to still set a `--readthedocs-font-size` and similar variables.
Merge branch 'humitos/footer-style' into agj/footer-style
| // | ||
| // NOTE: We can't include this on testing due to this error: | ||
| // Failed to set the 'adoptedStyleSheets' property on 'Document': Failed to convert value to 'CSSStyleSheet'. | ||
| if (!IS_TESTING) { |
There was a problem hiding this comment.
I think we have this pattern in a couple other places that might be worth removing as well 💯
There was a problem hiding this comment.
If we are going to follow this path, we should probably create a helper function that does this correctly for us so we don't have to include this block of code everywhere.
By the way, @agjohnson do you understand why this doesn't work on tests? Why the doctoolsStyleSheet is not an instance of CSSResult under testing? 🤷🏼
There was a problem hiding this comment.
I don't think we need to change this pattern elsewhere yet. I'm just fixing the import for our tests rather than masking this code from tests. I didn't handle the test failure in my last PR, this conditional was added afterwards.
The error comes from web-test-runner using Rollup and our builds using Webpack. Both these handle the import { styleSheet } from "flyout.css" by exporting differing classed objects.
There was a problem hiding this comment.
This works great for me. I tested it using
:root {
--readthedocs-flyout-font-size: 2rem;
}
and I can see the changes immediately.
I proposed an approach to use <addons>.defaults.css in #488 that we can include here (or I can do it in another PR) to have these CSS following a standardized pattern.
| // | ||
| // NOTE: We can't include this on testing due to this error: | ||
| // Failed to set the 'adoptedStyleSheets' property on 'Document': Failed to convert value to 'CSSStyleSheet'. | ||
| if (!IS_TESTING) { |
There was a problem hiding this comment.
If we are going to follow this path, we should probably create a helper function that does this correctly for us so we don't have to include this block of code everywhere.
By the way, @agjohnson do you understand why this doesn't work on tests? Why the doctoolsStyleSheet is not an instance of CSSResult under testing? 🤷🏼
|
I wouldn't use a separate, secondary CSS files for variable defaults for each addon unless we have to. I might even just prefer one CSS file if we're doing that. Here's an example of what it looks like if we author CSS variable defaults in each addon CSS file: This is just an example of what we can work towards, I don't think we need to hold up this PR or the underlying to continue working on this pattern. |
|
Cool, I'll merge this PR into mine and try to wrap mine as well so we can move forward. |
layer. Layers have less precedence than user CSS styles: https://developer.mozilla.org/en-US/docs/Web/CSS/@layer#descriptionWith the
@layer, user CSS can use:root { --readthedocs-flyout-font-size: 20px; }to override the defaults. There is no need to use specificity or inheritance order to override default variables.