Improve style sheet adoption ergonomics. #166

theengineear · 2024-02-02T21:42:45Z

One common pattern for element authors (now that import attributes enable folks to import css files directly) is to adopt imported style sheets into a shadow root at custom element initialization time.

This adds one static getters — styles. The goal is to make shadow root initialization as declarative as possible. Note that we still expose createRenderRoot so we don’t get in the way if folks need to do something more advanced (it’s also still the only way to forgo the creation of a shadow root at all).

Closes #52.

theengineear · 2024-02-02T21:47:45Z

x-element.js

+   * stylesheet file via import attributes. This has no effect if you are using
+   * the “host” as your render root (versus attaching a shadow root).
+   */
+  static get styleSheets() {


This is the crux of the change. We can fuss over the name here. I personally like cssStyleSheets, styleSheets, or styles as the options. But, I’m willing to be convinced otherwise 😸

import MyStylesAreTheBest from './path-to.css' with { type: 'css' }; import YourStyles from 'https://your.style/sheet.css' with { type: 'css' }; static get styles() { const rules = `:host { display: block }`; const inlineStyles = new CSSStyleSheet(); inlineStyles.replaceSync(rules); return [ MyStylesAreTheBest, YourStyles, inlineStyles, ]; }

theengineear · 2024-02-02T21:52:19Z

test/test-render-root.js

+
+class TestElement2 extends XElement {
+  static get styleSheets() {
+    // TODO: Replace with direct import of css file when better-supported in


You’ll have to use your imagination a bit until import attributes are better-supported. This is the pattern we’d be using though.

We no longer need to use our imagination :^)

We might be able to update these to import the CSS in directly. Assuming all our testing clients are using Chrome, that should probably work!.

… except for ESLint doesn’t dig this yet. So close!

theengineear · 2024-02-02T21:52:59Z

@klebba — This is more-or-less what I figured we’d do here. It’s pretty straightforward, just a way to declare things in isolation as an ergonomic benefit.

klebba · 2024-10-08T18:00:31Z

x-element.js

+  }
+
+  /**
+   * Declare an array of CSSSTyleSheet objects to adopt on the shadow root.


nit: typo STyle

klebba · 2024-10-08T18:07:22Z

x-element.js

@@ -68,7 +87,9 @@ export default class XElement extends HTMLElement {
   * E.g., setup focus delegation or return host instead of host.shadowRoot.
   */
  static createRenderRoot(host) {
-    return host.attachShadow({ mode: 'open' });
+    const shadowRoot = host.attachShadow(this.shadowRootInit);


I would recommend against the shadowRootInit concept — developers should simply override createRenderRoot to customize the render root init

klebba · 2024-10-08T18:12:38Z

x-element.js

@@ -68,7 +87,9 @@ export default class XElement extends HTMLElement {
   * E.g., setup focus delegation or return host instead of host.shadowRoot.
   */
  static createRenderRoot(host) {
-    return host.attachShadow({ mode: 'open' });
+    const shadowRoot = host.attachShadow(this.shadowRootInit);
+    shadowRoot.adoptedStyleSheets = this.styleSheets;


I recommend moving this out of createRenderRoot and instead moving it to the constructor — it would throw an error if there is no shadowRoot on the host

theengineear · 2024-10-08T19:57:06Z

x-element.js

+      } else {
+        throw new Error('Unexpected "styles" declared without a shadow root.');
+      }
+    }


Kind of a lot of lines dedicated to error-handling… but so it goes 🤷

theengineear · 2024-10-08T20:03:41Z

CHANGELOG.md

+
+### Fixed
+
+- The `map` function now works with properties / attributes bound across template contexts (#179).


Just adding this in the [Unreleased] section — forgot to request this earlier.

One common pattern for element authors (now that import attributes enable folks to import css files directly) is to adopt imported style sheets into a shadow root at custom element initialization time. This adds one static getter — `styles`. It’s still possible to do something more custom in `createRenderRoot`, but this adds a simple, declarative interface for the task of adding styles to a shadow root. Closes #52.

theengineear · 2024-10-16T04:07:56Z

test/test-styles.js

+  el.remove();
+});
+
+it('should only get styles _once_ per constructor', () => {


Importantly, this function is called once since it’s a static member, we follow our typical pattern — assume it can be set up once during static analysis and re-used for all instances.

klebba · 2024-10-18T20:35:25Z

x-element.js

@@ -21,6 +22,15 @@ export default class XElement extends HTMLElement {
    return XElement.defaultTemplateEngine;
  }

+  /**
+   * Declare an array of CSSStyleSheet objects to adopt on the shadow root.
+   * Note that a CSSStyleSheet object is the type returned when importing a


nit: an example syntax might be more useful here

I’ll add some additional commentary into the changes to use JSDoc 👌

klebba · 2024-10-18T20:36:24Z

x-element.js

@@ -534,7 +544,18 @@ export default class XElement extends HTMLElement {
    const computeMap = new Map();
    const observeMap = new Map();
    const defaultMap = new Map();
-    const { propertyMap } = XElement.#constructors.get(host.constructor);
+    const { styles, propertyMap } = XElement.#constructors.get(host.constructor);
+    if (styles.length > 0) {


nit: if (styles.length)

klebba · 2024-10-18T20:37:37Z

Looks awesome, nice work!

theengineear force-pushed the style-sheet-ergonomics branch from df8c01d to 2604001 Compare February 2, 2024 21:43

theengineear commented Feb 2, 2024

View reviewed changes

theengineear requested a review from klebba February 2, 2024 21:53

klebba reviewed Oct 8, 2024

View reviewed changes

x-element.js Outdated

}

/**

* Declare an array of CSSSTyleSheet objects to adopt on the shadow root.

Copy link

Collaborator

klebba Oct 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: typo STyle

klebba reviewed Oct 8, 2024

View reviewed changes

theengineear force-pushed the style-sheet-ergonomics branch 3 times, most recently from fc224a2 to 784cd5f Compare October 8, 2024 19:55

theengineear commented Oct 8, 2024

View reviewed changes

theengineear force-pushed the style-sheet-ergonomics branch from 784cd5f to 3661bb0 Compare October 8, 2024 20:02

theengineear commented Oct 8, 2024

View reviewed changes

theengineear force-pushed the style-sheet-ergonomics branch from 3661bb0 to 526c68f Compare October 16, 2024 04:00

theengineear commented Oct 16, 2024

View reviewed changes

klebba reviewed Oct 18, 2024

View reviewed changes

klebba approved these changes Oct 18, 2024

View reviewed changes

theengineear merged commit 827031d into main Oct 18, 2024
1 check passed

theengineear deleted the style-sheet-ergonomics branch October 18, 2024 21:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve style sheet adoption ergonomics. #166

Improve style sheet adoption ergonomics. #166

theengineear commented Feb 2, 2024 •

edited

Loading

theengineear Feb 2, 2024

klebba Oct 8, 2024 •

edited

Loading

theengineear Feb 2, 2024

theengineear Oct 8, 2024

theengineear Oct 8, 2024

theengineear commented Feb 2, 2024

klebba Oct 8, 2024

klebba Oct 8, 2024

klebba Oct 8, 2024

theengineear Oct 8, 2024

theengineear Oct 8, 2024

theengineear Oct 16, 2024

klebba Oct 18, 2024

theengineear Oct 18, 2024

klebba Oct 18, 2024

klebba commented Oct 18, 2024


		### Fixed

		- The `map` function now works with properties / attributes bound across template contexts (#179).

Improve style sheet adoption ergonomics. #166

Improve style sheet adoption ergonomics. #166

Conversation

theengineear commented Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

klebba Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theengineear commented Feb 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klebba commented Oct 18, 2024

theengineear commented Feb 2, 2024 •

edited

Loading

klebba Oct 8, 2024 •

edited

Loading