fix: simplify adopted stylesheets controller #441
Conversation
|
WalkthroughThe recent update involves a significant renaming and restructuring of the Changes
TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
❌ Deploy Preview for outlinejs failed.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (1)
- packages/controllers/adopted-stylesheets/src/adopted-stylesheets.ts (1 hunks)
Additional comments: 2
packages/controllers/adopted-stylesheets/src/adopted-stylesheets.ts (2)
- 8-12: The class
AdoptedStyleSheetsis well-documented, clearly stating its purpose and functionality. However, consider adding more detail to the documentation of thecssproperty to explain the distinction betweenglobalCSSandencapsulatedCSSmore clearly. This will enhance the readability and maintainability of the code.- 74-80: The
hostDisconnectedcallback currently takes no action. While this might be appropriate given the current implementation, consider if there's a need to clean up or remove applied stylesheets from the light DOM when the last instance of a component is removed from the document. This could help prevent potential memory leaks or style conflicts.
| // A set to track the stylesheets applied to the light DOM. | ||
| private static appliedLightDomStylesheets: Set<string> = new Set(); |
There was a problem hiding this comment.
Using a static property appliedLightDomStylesheets to track stylesheets applied to the light DOM is a good approach to prevent duplicate styles from being applied. However, ensure that this does not lead to memory leaks by holding references to styles that are no longer needed. Consider implementing a cleanup mechanism if necessary.
| private applyCssToDom(cssText: string, target: Document | ShadowRoot) { | ||
| if (target instanceof Document) { | ||
| const store = AdoptedStyleSheets.appliedLightDomStylesheets; | ||
|
|
||
| if (store.has(cssText)) { | ||
| // If the stylesheet has already been applied, no further action is required. | ||
| return; | ||
| } | ||
| store.add(cssText); // Store the stylesheet with the provided key. | ||
| } | ||
| this.adoptedSheet = | ||
| AdoptedStylesheets.styleSheetMap.get(cssText) || new CSSStyleSheet(); | ||
|
|
||
| // Create a new stylesheet and replace its contents with the provided CSS text. | ||
| const sheet = new CSSStyleSheet(); | ||
| sheet.replaceSync(cssText); | ||
|
|
||
| // Apply the stylesheet to the target's adoptedStyleSheets. | ||
| target.adoptedStyleSheets = [...target.adoptedStyleSheets, sheet]; |
There was a problem hiding this comment.
The method applyCssToDom is well-implemented, with clear separation of logic for applying styles to the Document and ShadowRoot. However, using CSSStyleSheet and adoptedStyleSheets requires the Shadow DOM to be enabled and might not be supported in all browsers. Ensure that there's a fallback mechanism or a polyfill for browsers that do not support these features.
| if (this.css.globalCSS) { | ||
| this.applyCssToDom(this.css.globalCSS, document); // Apply global CSS to the document if it exists. | ||
| } | ||
|
|
||
| // Apply encapsulated CSS to the host's shadow root if it exists. | ||
| if (this.css.encapsulatedCSS && this.host.shadowRoot) { | ||
| this.applyCssToDom(this.css.encapsulatedCSS, this.host.shadowRoot); // Apply encapsulated CSS to the host's shadow root if it exists. |
There was a problem hiding this comment.
The hostConnected lifecycle callback efficiently applies global and encapsulated CSS to the appropriate DOM targets. This implementation aligns with the PR's objective to improve the management of CSS styles. Ensure that the CSS strings are sanitized or validated to prevent potential security issues like CSS injection.
Summary by CodeRabbit
AdoptedStylesheetsclass toAdoptedStyleSheetsand updated its properties and methods for improved management of global and encapsulated CSS styles in Lit elements.