-
Notifications
You must be signed in to change notification settings - Fork 943
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
Ensure template given to ShadyCSS is the one rendered by lit-html #502
Conversation
This ensures ShadyCSS can update the style in the template that will be rendered by lit-html. ShadyCSS needs to do this, for example, when the set of properties changes in any `@apply`'s. Fixes lit/lit-element#117
src/lib/shady-render.ts
Outdated
shadyRenderSet.add(scopeName); | ||
// Move styles out of rendered DOM and store. | ||
const styleFragment = document.createDocumentFragment(); | ||
Array.from(fragment.querySelectorAll('style')).forEach((s: Element) => { |
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'd use a for loop... not a lot of reason to allocate the array.
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.
Done.
src/lib/shady-render.ts
Outdated
}); | ||
// Remove styles from nested templates in this scope and put them | ||
// into the "root" template passed in as `template`. | ||
removeStylesFromLitTemplates(scopeName); |
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.
could this be renamed to be more clear that it's moving styles?
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.
It is named correctly, but the comment is for the next two lines. Moved the comments to make more clear.
src/lib/shady-render.ts
Outdated
// Remove styles from nested templates in this scope and put them | ||
// into the "root" template passed in as `template`. | ||
removeStylesFromLitTemplates(scopeName); | ||
insertNodeIntoTemplate( |
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.
Is this the big change here? I'm having a little bit of a hard time seeing the change in behavior.
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.
Added a comment. Yes, the key bit is that we hand ShadyCSS
a template that will actually be rendered by lit-html
. This way it can update the style inside it and affect subsequent lit-html
renderings.
@justinfagnani PTAL |
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 for the updated comments!
…t#502) This ensures ShadyCSS can update the style in the template that will be rendered by lit-html. ShadyCSS needs to do this, for example, when the set of properties changes in any `@apply`'s.
This ensures ShadyCSS can update the style in the template that will be rendered by lit-html. ShadyCSS needs to do this, for example, when the set of properties needed in any
@apply
changes.Fixes lit/lit-element#117