-
Notifications
You must be signed in to change notification settings - Fork 87
feat: implement initial version of CSSInjectionMixin #8934
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
base: main
Are you sure you want to change the base?
Conversation
f897fe5
to
b9e5a3d
Compare
@tomivirkki Marked you as a reviewer for this PR, it's optional but your feedback would be highly appreciated. |
// Add the new stylesheet at the beginning of the adoptedStyleSheets array | ||
component.shadowRoot.adoptedStyleSheets = [styleSheet, ...adoptedStyleSheets]; |
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 relies on the assumption that the new base styles will use @layer
but maybe we could improve this:
- Update
finalizeStyles()
method inThemableMixin
to store styles fromstatic get styles()
separately - Have some unified helper that would then apply styles in the right order when updating instance styles:
- first, styles from
static get styles()
- next, styles injected by
CSSInjector
- finally, styles added with
registerStyles()
- first, styles from
If this sounds reasonable, I will prototype this in a separate branch.
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.
UPD: implemented this logic in a new commit and updated tests accordingly.
bcd96d8
to
2c34760
Compare
} | ||
|
||
// Observe custom property that would trigger injection for this class | ||
this.#styleObserver.observe(this.#rootHost, cssInjectPropName); |
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 this also be unobserved in componentDisconnected
?
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 was thinking that maybe we could unobserve root host when there are no more components added to it (especially if it's not the document but a shadow root host). This could be implemented in a follow-up PR.
|
import { CSSInjector } from './src/css-injector.js'; | ||
|
||
/** | ||
* @type {string[]} |
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.
nit: Set<string>
Description
Part of vaadin/platform#7453
Added
CSSInjectionMixin
that allows injecting CSS into components using the following syntax:Above styles can be injected either in a document or in a shadow root of some parent component.
Type of change
Note
Can be tested in
/dev/lumo-injection/
pages using this branch:https://github.com/vaadin/web-components/tree/proto/css-injection