Theme architecture proposal #42

heff · 2021-07-29T04:27:59Z

I was thinking through how we might host and load themes, and I landed on using custom elements to do it.

Custom elements:

Have a mechanism for template variables (slots)
Have a global registry, accessible to the shadow and light doms on the page (customElements.define). This would be hard to recreate otherwise.
Have a mechanism for knowing when the template is loaded (customElements.whenDefined)

Hoping to get feedback.

I see two ways they might be used.

1. Used directly in the HTML

<media-theme-netflix>
  <video slot="media">
</media-theme-netflix>

2. Access the template string to clone into the shadow dom of a wrapping player

<my-player theme="netflix">
  <video slot="media">
</my-player>

Video player constructor

constructor() {
    const shadow = this.attachShadow({ mode: 'open' });

    customElements.whenDefined(themeName).then(()=>{
      const theme = customElements.get(themeName);
      const template = themeEl.template;
      shadow.innerHTML = template;
    });
}

vercel · 2021-07-29T04:28:01Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/mux/media-chrome/J2FjGjWfiPwtW8QQBntmp1jcgrNG
✅ Preview: https://media-chrome-git-themes-mux.vercel.app

heff · 2021-07-30T00:29:06Z

themes/media-theme-netflix.js

+
+<media-controller>
+
+  <slot name="media" slot="media"></slot>


This is the thing that feels nuckin futs. Chaining slots. But it works!

Whoa.

So from the outside, an application does this:

<media-theme-netflix> <video slot="media"></video> </media-theme-netflix>

That "media" slot gets passed through into the "media" slot for media-controller. Pretty nuckin futs

heff · 2021-08-20T04:12:35Z

@dylanjha @mmcc @cjpillsbury Speak up if you have an opinion on this, otherwise I'm gonna move forward with it.

dylanjha

I'm in favor of this approach with <media-theme-xxx> elements. Easy to understand what's going on and each of the <media-theme-xxx> themes provide a great example.

The one question I have is:

What about in this Netflix example when the theme contains other variables like videoTitle? -- Would the media-theme-netflix specify other attributes like <media-theme-netflix video-title="Breaking Bad" video-episode="Episode 2">

dylanjha · 2021-09-01T16:30:18Z

themes/media-theme-netflix.js

+
+<media-controller>
+
+  <slot name="media" slot="media"></slot>


Whoa.

So from the outside, an application does this:

<media-theme-netflix> <video slot="media"></video> </media-theme-netflix>

That "media" slot gets passed through into the "media" slot for media-controller. Pretty nuckin futs

heff · 2021-09-01T18:34:17Z

What about in this Netflix example when the theme contains other variables like videoTitle

Good call that this needs to be clear as part of this.

Slots are the first answer for me. Themes may have lots of use cases for embedding custom content, and using attributes might feel overloaded. Slots are a custom element's answer to content "variables". And compared to attributes, updates to the slot contents are automatically handled.

<media-theme-abc>
  <video slot="media"></video>
  <span slot="media-title">My great title</span>
</media-theme-abc>

A second answer for things like title specifically is potentially defining media metadata elements inside a media element. I think I mentioned this in another issue somewhere.

<media-theme-abc>
  <video slot="media">
    <media-metadata-title>My great title</media-metadata-title>
  </video>
</media-theme-abc>

With that, if you swap the video, for example as part of playlist, the metadata comes/goes with it. Doesn't even need to be a real custom element, I think. A downside is handling changes to those elements. Might require yet more mutation observers. But upside is it's compatible with the native audio/video elements, which hide that content by default but still make it available to javascript.

So it might end up being both. For example, the <media-title> UI element may have a media-title="My title" attribute that can be set by <media-controller> (the title being read from the <media-metadata-title> element). And the also you can have a slot within the theme that is child content and overrides the attribute value, <media-title><slot name="media-title"></slot></media-title> which overrides the attribute value.

Does that make sense? Any thoughts or ideas on that?

dylanjha · 2021-09-03T18:30:13Z

@heff I love using slots for this.

I'm on board with the idea that any particular "theme" like <media-theme-netflix> can define a few of it's own slots. That approach makes a lot of sense.

cjpillsbury

mostly nits.

cjpillsbury · 2021-09-16T20:34:36Z

src/js/media-controller.js

+        document[fullscreenApi.exit]();
+
+        // Shadow root throws an error for this function
+        // this.getRootNode()[fullscreenApi.exit]();


Do we know why we were using this.getRootNode() here originally? will this work for iframe embeds? Noticed we were using getRootNode() in several places. If this is safe, should we be changing it in other locations as well?

It seems like it's gonna be context specific, depending on if we need the shadowRoot when that's the element's context and if it supports the needed APIs.

cjpillsbury · 2021-09-16T20:36:35Z

themes/example.html

+</media-theme-netflix>
+
+</body>
+</html>


nit: missing newline for EOF

cjpillsbury · 2021-09-16T20:37:05Z

themes/example.html

@@ -0,0 +1,18 @@
+<html>


nit: We should probably keep "examples" separate from "source"

cjpillsbury · 2021-09-16T23:28:37Z

themes/media-theme-netflix.js

@@ -0,0 +1,164 @@
+import MediaTheme from './media-theme.js';


Fine for now, but there are several inconsistencies with the current browser-based netflix player. Should make followup issue(s) to clean up. Also may want to consider splitting out the part that sizes this to full width from the rest of the theme.

cjpillsbury · 2021-09-16T23:32:11Z

themes/media-theme-netflix.js

+
+class MediaThemeNetflix extends MediaTheme {
+  constructor(options={}) {
+    options = Object.assign({


nit: no need to mutate options (may cause unexpected side effects for implementors). consider instead simply inline object spread, a la:

super(template, { /* allow ...defaultOptions, */ ...options });

How is options getting mutated? Either way I'm good with the spread.

cjpillsbury · 2021-09-16T23:34:07Z

themes/media-theme.js

+  constructor(template=``, options={}) {
+    super();
+
+    options = Object.assign({


see comment ☝️

cjpillsbury · 2021-09-16T23:37:35Z

src/js/media-container.js

+    let media = this.querySelector(':scope > [slot=media]');
+
+    // Chaining media slots for media templates
+    if (media.nodeName == 'SLOT') media = media.assignedElements({flatten:true})[0];


Fine for now. May be some weird edge cases that are missed, but we can file followup issues if/when they show up and are a priority.

Such a pessimist. :) It's like you've had to work in the browser before or something.

heff · 2021-10-17T05:17:17Z

Cleaned up and finally merging in.

dylanjha · 2021-10-18T05:02:05Z

yooooo 🙌🏼 🎉

heff added 2 commits July 28, 2021 21:10

First pass at themes

b93c82c

Simplified example.html for first review

5d9773c

heff commented Jul 30, 2021

View reviewed changes

Merge branch 'main' into themes

0996772

dylanjha approved these changes Sep 1, 2021

View reviewed changes

cjpillsbury requested changes Sep 16, 2021

View reviewed changes

heff added 3 commits October 2, 2021 11:04

Updating themes

d9a7318

Merge branch 'main' into themes

5173fd0

Cleaning, merging, addressing comments

6baf09b

vercel bot deployed to Preview October 17, 2021 05:14 View deployment

heff merged commit f6132b6 into main Oct 17, 2021

dylanjha deleted the themes branch October 18, 2021 05:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Theme architecture proposal #42

Theme architecture proposal #42

heff commented Jul 29, 2021

vercel bot commented Jul 29, 2021 •

edited

Loading

heff Jul 30, 2021

dylanjha Sep 1, 2021

heff commented Aug 20, 2021

dylanjha left a comment

dylanjha Sep 1, 2021

heff commented Sep 1, 2021

dylanjha commented Sep 3, 2021

cjpillsbury left a comment

cjpillsbury Sep 16, 2021

heff Oct 17, 2021

cjpillsbury Sep 16, 2021

cjpillsbury Sep 16, 2021

heff Sep 17, 2021

cjpillsbury Sep 16, 2021

cjpillsbury Sep 16, 2021

heff Oct 17, 2021

cjpillsbury Sep 16, 2021

cjpillsbury Sep 16, 2021

heff Oct 17, 2021

heff commented Oct 17, 2021

dylanjha commented Oct 18, 2021

Theme architecture proposal #42

Theme architecture proposal #42

Conversation

heff commented Jul 29, 2021

1. Used directly in the HTML

2. Access the template string to clone into the shadow dom of a wrapping player

vercel bot commented Jul 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heff commented Aug 20, 2021

dylanjha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heff commented Sep 1, 2021

dylanjha commented Sep 3, 2021

cjpillsbury left a 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

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

heff commented Oct 17, 2021

dylanjha commented Oct 18, 2021

vercel bot commented Jul 29, 2021 •

edited

Loading