AdoptedStyleSheets Update: Attach styles to document or this.shadowRoot
#431
Conversation
…flexible. BREAKING CHANGE
|
❌ Deploy Preview for outlinejs failed.
|
|
Important Auto Review SkippedReview was skipped due to path filters Files ignored due to path filters (1)
WalkthroughThe update introduces a refined approach to managing styles within web components through 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 (
|
![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 (2)
- packages/controllers/adopted-stylesheets/README.md (2 hunks)
- packages/controllers/adopted-stylesheets/src/adopted-stylesheets.ts (1 hunks)
Additional comments: 2
packages/controllers/adopted-stylesheets/README.md (2)
- 33-33: The documentation correctly reflects the updated constructor signature with the
rootparameter. This aligns with the changes in the source code.- 41-101: The usage examples in the README have been updated to demonstrate the new functionality of attaching stylesheets to both the document and shadow roots. The examples are clear and correctly show the use of the
rootparameter in the constructor. However, ensure that the examples are tested to confirm that they work as intended, especially since the direct assignment toadoptedStyleSheetsin the source code is not possible.
packages/controllers/adopted-stylesheets/src/adopted-stylesheets.ts
Outdated
Show resolved
Hide resolved
packages/controllers/adopted-stylesheets/src/adopted-stylesheets.ts
Outdated
Show resolved
Hide resolved
packages/controllers/adopted-stylesheets/src/adopted-stylesheets.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- packages/controllers/adopted-stylesheets/src/adopted-stylesheets.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/controllers/adopted-stylesheets/src/adopted-stylesheets.ts
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (2)
- packages/controllers/adopted-stylesheets/README.md (2 hunks)
- packages/controllers/adopted-stylesheets/src/adopted-stylesheets.ts (1 hunks)
Additional comments: 5
packages/controllers/adopted-stylesheets/src/adopted-stylesheets.ts (3)
- 1-10: The import statement and class definition are correct and follow TypeScript conventions.
- 69-75: Ensure that the logic in
hostConnectedcorrectly updates theadoptedStyleSheetsarray without directly mutating it, as this follows best practices for immutability.- 85-87: The filtering logic in
hostDisconnectedis correct and efficiently removes the stylesheet from theadoptedStyleSheetsarray.packages/controllers/adopted-stylesheets/README.md (2)
- 9-19: The installation instructions are clear and correctly specify commands for Yarn, NPM, and PNPM package managers.
- 2-27: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [24-28]
The overview provides useful links to MDN Web Docs for further reading on
adoptedStyleSheets. This is helpful for users seeking more in-depth information.
packages/controllers/adopted-stylesheets/src/adopted-stylesheets.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- packages/controllers/adopted-stylesheets/src/adopted-stylesheets.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/controllers/adopted-stylesheets/src/adopted-stylesheets.ts
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
package.jsonis excluded by:!**/*.jsonpackages/controllers/adopted-stylesheets/package.jsonis excluded by:!**/*.json
Files selected for processing (1)
- packages/controllers/adopted-stylesheets/README.md (1 hunks)
Additional comments: 3
packages/controllers/adopted-stylesheets/README.md (3)
- 41-41: The constructor's documentation correctly reflects the new
rootparameter, enhancing the controller's flexibility. This change aligns with the PR objectives to allow attaching stylesheets to either the main document or a shadow root.- 104-105: The use of
this.shadowRootto conditionally create an instance ofAdoptedStylesheetsfor encapsulated styles is a good practice. It ensures styles are only attached if a shadow root is available, aligning with the modern standards for web components. This example effectively demonstrates the flexibility introduced by therootparameter.- 121-121: The additional information section provides valuable resources for further reading on adopted stylesheets. Including links to MDN Web Docs is helpful for developers seeking more in-depth knowledge. This section is well-curated and relevant.
| To install the new package, use the following command, specific to your package manager: | ||
|
|
||
| ### With Yarn | ||
|
|
||
| > For more information on using Yarn, visit the [Yarn documentation](https://classic.yarnpkg.com/en/docs/). | ||
|
|
||
| ```bash | ||
| yarn add @phase2/outline-adopted-stylesheets-controller | ||
| ``` | ||
|
|
||
| ## Overview | ||
| ### With NPM | ||
|
|
||
| Adopted stylesheets are a method to apply styles to a document or a shadow root. They are a part of the CSS Shadow Parts specification. Unfortunately, the original documentation link is no longer available. However, you can find more information about adopted stylesheets and their usage in web components in the following resources: | ||
| > For more information on using NPM, visit the [NPM documentation](https://docs.npmjs.com/). | ||
|
|
||
| - [MDN Web Docs: Document adoptedStyleSheets](https://developer.mozilla.org/en-US/docs/Web/API/Document/adoptedStyleSheets): This documentation provides an in-depth look at the `adoptedStyleSheets` property of the `Document` interface. | ||
| - [MDN Web Docs: ShadowRoot adoptedStyleSheets](https://developer.mozilla.org/en-US/docs/Web/API/ShadowRoot/adoptedStyleSheets): This documentation provides an in-depth look at the `adoptedStyleSheets` property of the `ShadowRoot` interface. | ||
| ```bash | ||
| npm i --save-dev @phase2/outline-adopted-stylesheets-controller | ||
| ``` | ||
|
|
||
| ### With PNPM | ||
|
|
||
| > For more information on using PNPM, visit the [PNPM documentation](https://pnpm.io/usage). | ||
|
|
||
| ```bash | ||
| pnpm add --save-dev @phase2/outline-adopted-stylesheets-controller | ||
| ``` |
There was a problem hiding this comment.
The installation instructions are clear and cover multiple package managers (Yarn, NPM, PNPM). However, for NPM and PNPM, the flag --save-dev is used, which suggests these dependencies are only needed during development. This might not be accurate for a library intended to be used in production. Consider using --save for NPM and removing --save-dev for PNPM to correctly reflect that this package should be part of the production dependencies.
- npm i --save-dev @phase2/outline-adopted-stylesheets-controller
+ npm i @phase2/outline-adopted-stylesheets-controller
- pnpm add --save-dev @phase2/outline-adopted-stylesheets-controller
+ pnpm add @phase2/outline-adopted-stylesheets-controllerCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| To install the new package, use the following command, specific to your package manager: | |
| ### With Yarn | |
| > For more information on using Yarn, visit the [Yarn documentation](https://classic.yarnpkg.com/en/docs/). | |
| ```bash | |
| yarn add @phase2/outline-adopted-stylesheets-controller | |
| ``` | |
| ## Overview | |
| ### With NPM | |
| Adopted stylesheets are a method to apply styles to a document or a shadow root. They are a part of the CSS Shadow Parts specification. Unfortunately, the original documentation link is no longer available. However, you can find more information about adopted stylesheets and their usage in web components in the following resources: | |
| > For more information on using NPM, visit the [NPM documentation](https://docs.npmjs.com/). | |
| - [MDN Web Docs: Document adoptedStyleSheets](https://developer.mozilla.org/en-US/docs/Web/API/Document/adoptedStyleSheets): This documentation provides an in-depth look at the `adoptedStyleSheets` property of the `Document` interface. | |
| - [MDN Web Docs: ShadowRoot adoptedStyleSheets](https://developer.mozilla.org/en-US/docs/Web/API/ShadowRoot/adoptedStyleSheets): This documentation provides an in-depth look at the `adoptedStyleSheets` property of the `ShadowRoot` interface. | |
| ```bash | |
| npm i --save-dev @phase2/outline-adopted-stylesheets-controller | |
| ``` | |
| ### With PNPM | |
| > For more information on using PNPM, visit the [PNPM documentation](https://pnpm.io/usage). | |
| ```bash | |
| pnpm add --save-dev @phase2/outline-adopted-stylesheets-controller | |
| ``` | |
| To install the new package, use the following command, specific to your package manager: | |
| ### With Yarn | |
| > For more information on using Yarn, visit the [Yarn documentation](https://classic.yarnpkg.com/en/docs/). | |
| ```bash | |
| yarn add @phase2/outline-adopted-stylesheets-controller |
With NPM
For more information on using NPM, visit the NPM documentation.
npm i @phase2/outline-adopted-stylesheets-controllerWith PNPM
For more information on using PNPM, visit the PNPM documentation.
pnpm add @phase2/outline-adopted-stylesheets-controller
</details>
<!-- suggestion_end -->
<!-- This is an auto-generated comment by CodeRabbit -->
| GlobalStylesheets: AdoptedStylesheets | undefined = new AdoptedStylesheets( | ||
| this, // The host that this controller is associated with | ||
| globalStyles, // A string that contains the CSS styles to be adopted | ||
| document, // The root where the stylesheet will be adopted |
There was a problem hiding this comment.
In the example of attaching a global stylesheet, the this keyword is incorrectly passed as the first argument to the AdoptedStylesheets constructor. This seems to be a mistake since the constructor expects a CSSResult and a root (Document or ShadowRoot) as parameters. The this keyword in this context refers to the component instance, not a stylesheet or root. This needs correction to match the constructor's expected parameters.
- GlobalStylesheets: AdoptedStylesheets | undefined = new AdoptedStylesheets(
- this, // The host that this controller is associated with
- globalStyles, // A string that contains the CSS styles to be adopted
- document, // The root where the stylesheet will be adopted
+ GlobalStylesheets: AdoptedStylesheets | undefined = new AdoptedStylesheets(globalStyles, document);Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| GlobalStylesheets: AdoptedStylesheets | undefined = new AdoptedStylesheets( | |
| this, // The host that this controller is associated with | |
| globalStyles, // A string that contains the CSS styles to be adopted | |
| document, // The root where the stylesheet will be adopted | |
| GlobalStylesheets: AdoptedStylesheets | undefined = new AdoptedStylesheets(globalStyles, document); |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (2)
- packages/controllers/adopted-stylesheets/README.md (1 hunks)
- packages/controllers/adopted-stylesheets/src/adopted-stylesheets.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/controllers/adopted-stylesheets/README.md
Additional comments: 3
packages/controllers/adopted-stylesheets/src/adopted-stylesheets.ts (3)
- 44-47: The constructor's signature has been correctly updated to include
ReactiveControllerHost,cssText, and an optionalrootparameter. This aligns with the PR objectives for more flexible stylesheet management.- 69-75: The
hostConnectedmethod correctly adopts the CSSStyleSheet object into the root's adopted stylesheets if not already included. This implementation is efficient and aligns with modern browser standards for managing styles.- 85-87: The
hostDisconnectedmethod properly removes the CSSStyleSheet object from the root's adopted stylesheets if included. This ensures clean-up and prevents potential memory leaks, aligning with best practices for dynamic style management.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- packages/controllers/adopted-stylesheets/README.md (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/controllers/adopted-stylesheets/README.md
Summary by CodeRabbit
AdoptedStylesheetscontroller for improved handling of CSSStyleSheet objects and connection lifecycle.createRenderRootmethod in theAdoptedStylesheetscontroller.