Skip to content
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

Convert to Lit Element #2

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Convert to Lit Element #2

wants to merge 3 commits into from

Conversation

davatron5000
Copy link
Owner

@davatron5000 davatron5000 commented Jan 20, 2021

Moving <podcast-player> over to LitElement. Most of the a11y stuff was done under the supervision of @feather for the ShopTalk Show site. But I have questions! And I'd like feedback! Specifically around the best way to allow passing necessary customizations and styling down to the component.

Work done so far.

  • [DX] Convert to Lit Element
  • [A11y] Convert progress to range input
  • [A11y] Add A11y labels
  • [Feature] Add TimeJump
  • [Feature] Add 30s rewind and 30s ff
  • [Feature] Add CSS grid-area properties for custom styling
  • [API Change] Now requires a child <audio> element for progressively enhancing

Todo:

  • Move to package dependency, not unpkg
  • Add fucking build tools
  • Fix a few bugs where this.currentTime doesn't reset after the track has finished.
  • Add CSS variables for color theming
  • Fix hiccup in the input range smoothness

Questions:

  1. Is there a better way to allow custom styles? I've already added named grid-area properties, which might be a good hook
  2. Is there a better way to allow custom icons? There's a spritesheet with icons already, but I quickly imagine lots of situations where people would want to include their own custom icons.
  3. Is it possible to disable styles() entirely?

@feather
Copy link

feather commented Jan 20, 2021

Is there a better way to allow custom icons? There's a spritesheet with icons already, but I quickly imagine lots of situations where people would want to include their own custom icons.

@davatron5000 -- not sure about the short term, but in the longer term the Personalization Semantics Content Module work is what you're looking for.

The data-action attribute is what holds promise for that type of customization. At least, that would provide a standard way of doing so for an END USER. You might be talking about a person implementing... In which case, it might make sense to leverage data-action as that would allow a hook for the implementer, but also an end user that wants to customize as well.

@Westbrook
Copy link

There's been some interesting talk about the level of styling you can adopt in a pattern (regardless of implementation) lately and one path could be that you litter vend multiple levels. In that was you could ship PodcastPlayerBase with no styling, PodcastPlayer with basic styling, and then PodcastPlayerFancy etc with very custom styling. If you move customElements.define to another entry as outlined in https://open-wc.org/guides/developing-components/publishing/ users could then choose which they'd like to use and/or customize their own PodcastPlayerCustom from there. It's an interest concept, especially as far as scale of customizations go, and is somewhat similar to what you see in https://lion-web-components.netlify.app/?path=/story/forms-combobox-extensions--material-design, IIRC.

CSS Custom Properties are covering more controlled customizations. If you're not leveraging a "system", I worry that they can be pretty constraining. However, having the custom properties the base of the system like --podcast-player-rhythm and then use it to calculate things or --podcast-player-color-primary or --podcast-player-color-secondary etc. gives you a nice way to keep thins in that system. Ensuring that system aligns perfectly with the system outside of your element can sometimes cause friction. I know that the team behind Pattern Fly Elements spent a lot of time clarifying their property naming system to make the way you work a system of custom properties into a local customization of an element that is very nice. They document it pretty thoroughly in the "Notes" section of these stories: https://patternflyelements.com/storybook

On the surface of icons, I wonder if you can slot in a sprite sheet there? Likely not in the right DOM tree to be leveraged, but that would be quite cool. Using <slot><svg style="display: none;">...</svg></slot> so that a new slotted spite sheet could use the same icon names and customize their delivery would be so cool, please check that out and write it up if it actually works!! Otherwise, you could do something like the following for more explicit control:

<button class="button-mute" aria-label="Mute"  style="grid-area: mute" @click="${this.mute}">
          <slot name="unmuted-icon"><svg class="unmuted">
            <use xlink:href="#icon-speaker-unmuted"></use>
          </svg></slot>
          <slot name="muted-icon"><svg class="muted"><use xlink:href="#icon-speaker-muted"></use></svg></slot>
        </button>

It feels heavy to need to do each of these very directly, and positions you to need to do so many icons individually, which might not be fun for consumer. Though if you could slot a sprit sheet and then slot a <use xlink:href="#icon-speaker-unmuted"></use> would be very interesting, e.g. : <svg class="muted"><slot name="muted-icon"><use xlink:href="#icon-speaker-muted"></use></slot></svg> so that consumers could also be customizing the icons refs...

The one piece here that you've not brought up is the possibility to use CSS Parts. In that way you could skip local styles and then require consumers to fully style specific parts of the element from the outside, like you see with pseudo elements in form controls natively. In that was you could surface the 9 or so elements that users might want to control (play, pause, rewind, fast forward, time, range, duration, speed, unmute, mute, etc) or less, as some of these might not need full control for users to take control of each other these things on their own.

So many options, which is maybe the bigger problem here, sorry to drop so many non-answers on you. Happy to chat some more as you continue to finalize your decisions on this.

@Matsuuu
Copy link

Matsuuu commented Jan 20, 2021

To start off: Nice port!

I'm not too shabby with a11y and customizability, but I'll share a small change that I think could ease the code a bit:

You could use LitElement's provided ability to reflect properties to attributes, and control the playing and muted -statuses that way.

I created a fork with just that functionality, if you want to check it out, and consider it.

Matsuuu@2b315e2

@davatron5000
Copy link
Owner Author

@Matsuuu This looks great! If you want to PR against this branch, I can merge in.

@Westbrook Thanks for the feedback. I ❤️ the spritesheet slot idea. I'm going to take a stab at that. And if that works, then try the custom <use> slot. But the spritesheet slot gets things pretty far probably.

@feather First time hearing about Personalization Semantics. But should be easy to layer in. Is there any AT out there that has this yet to QA against whether or not I did it right?

Move from classes to attributes
@davatron5000 davatron5000 self-assigned this Jan 21, 2021
@feather
Copy link

feather commented Jan 21, 2021

@feather First time hearing about Personalization Semantics. But should be easy to layer in. Is there any AT out there that has this yet to QA against whether or not I did it right?

Not really, no 🙂 This is from work in progress and isn’t likely on the radar of any AT vendor yet, and the main use case will be people that create custom browser extensions/user styles for their own use. Think “I want all the search buttons that I ever see on any web page to use a set of binoculars instead of a magnifying glass” and “I want to make all rewind and ffwd buttons look like this << >> instead of the round arrow circley ones we see everywhere”

So nothing to do right quite yet - just wanted them to be on your radar so that you could plan for that eventuality... no reason not to adopt their naming conventions as you implement.

Copy link

@justinfagnani justinfagnani left a comment

Choose a reason for hiding this comment

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

Cool element. hope my comments are useful!

LitElement,
html,
css,
} from "https://unpkg.com/[email protected]/lit-element.js?module";

Choose a reason for hiding this comment

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

I would urge you not to hard code this in. It means a user can't control where they get lit-element from, can't serve it from their own server, and guarantees duplication if the use lit-element anywhere else. You also can't run local tests, you have to load from the network.

I would import from just lit-element and if you want to produce a bundle that auto-loads from unpkg, you could use Rollup to build a single bundle or one that resolves to unpkg.

class PodcastPlayer extends LitElement {
static get properties() {
return {
currentTime: { type: String },

Choose a reason for hiding this comment

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

fyi, {type: String} is the default. You can just do {}

window.addEventListener("hashchange", this.timeJump.bind(this), false);
}

handleLoadedMetadata() {

Choose a reason for hiding this comment

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

I would use an arrow function here and not .bind()

handLoadedMetadata = () => {...};

and

this.audio.addEventListener("timeupdate", this.handleTimeUpdate);

it'll make it easier to remove listeners

super();

// HTMLAudioElement
this.audio = this.querySelector("audio");

Choose a reason for hiding this comment

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

Since children can change I would do all this setup work in a slotchange event listener.

In particular, this.querySelector("audio") will return null if this element is created programmatically:

const el = document.createElement('podcast-player');
const audio = document.createElement('audio');
el.append(audio);

render() {
// TODO: Make icons, button text, button labels overrideable?
return html`
<slot></slot>

Choose a reason for hiding this comment

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

Do @slotchange=${this._onSlotChange} and you can setup everything for the audio element there, and it'll work if the audio element isn't present when this element is constructed.

}
}

parseTime(str) {

Choose a reason for hiding this comment

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

Since this doesn't access any instance state it can be a top-level function rather than an instance method.

var seconds = sec_num - hours * 3600 - minutes * 60;

if (hours < 10) {
hours = "0" + hours;

Choose a reason for hiding this comment

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

not sure this is much better, but padStart is p nice:

hours = `${hours}`.padStart(2, '0')

this.currentSpeedIdx + 1 < this.speeds.length
? this.currentSpeedIdx + 1
: 0;
this.audio.playbackRate = this.speeds[this.currentSpeedIdx];

Choose a reason for hiding this comment

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

I would try to make this more reactive by separating setting this element's state from reflecting that state into the DOM. So keep the this.currentSpeedIdx assignment here, but implement updated():

updated(changedProperties) {
  if (changedProperties.has('currentSpeedIdx') {
    this.audio?.playbackRate = this.speeds[this.currentSpeedIdx];
  }
}

That way if someone sets currentSpeedIdx externally the change will propagate.

}

mute() {
this.audio.muted = !this.audio.muted;

Choose a reason for hiding this comment

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

since this.audio could be null, a lot of these methods need either optional chaining on that property access, or should be guarded at the top.

Not sure your views on TypeScript, but if you marked audio as optional, it'd tell you when you need to guard access to it.

}

render() {
// TODO: Make icons, button text, button labels overrideable?

Choose a reason for hiding this comment

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

There's a few ways you can do this:

  • Slots inside the buttons
  • Slots outside the buttons (much more complex)
  • render props
  • overridable methods

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants