Add type annotation to the styles property of components to allow inherited styles

[sonntag-philipp]

What issue are you having?

I would like to create some components of my own on top of the Shoelace components.

To do this I already created an derivation of the SlButton class which looks like this:

@customElement("my-button")
export default class MyButton extends SlButton {
  static styles = [
    SlButton.styles,
    css`
      :host::part(base) {
        text-transform: uppercase;
      }
    `,
  ];
}

My main goal was to add some styles for now and some functionality in the future. To add styles to the derived component I followed the documentation of Lit.

This resulted in an type check error for my components. I have to implement the static styles property as a CSSResultGroup to add my own styles. This makes my CSSResultGroup property not compatible with the CSSResult used in the SlButton base class.

Describe the solution you'd like

Excerpt from the Google Lit documentation:

When writing components intended to be subclassed in TypeScript, the static styles field should be explicitly typed as CSSResultGroup to allow flexibility for users to override styles with an array:

If you want me to help I'm really happy to add the type annotations myself and submit a PR :)

I would modify the components like this:

@customElement('sl-button')
export default class SlButton extends LitElement {
  static styles: CSSResultGroup = styles;

  @query('.button') button: HTMLButtonElement | HTMLLinkElement;

Which would result in a declaration like this:

import type { CSSResultGroup } from 'lit';
import { LitElement } from 'lit';
import '../../components/spinner/spinner';
export default class SlButton extends LitElement {
    static styles: CSSResultGroup;

(Already tested on my local environment)

Describe alternatives you've considered

An alternative would be to write derivations like this:

@customElement("my-button")
export default class MyButton extends SlButton {
  static styles = [
    SlButton.styles,
    css`
      :host::part(base) {
        text-transform: uppercase;
      }
    `,
  ] as any as CSSResult;
}

I don't think I have to go further into detail why I don't really like this solution as it effectively makes the type validation for this property useless.

Just a little words on the side:
Thank you for this amazing project. The Shoelace library is the best web component library I've ever used by far. I would be really happy to contribute by solving this issue.

[claviska]

@customElement('sl-button')
export default class SlButton extends LitElement {
 static styles: CSSResultGroup = styles;

 @query('.button') button: HTMLButtonElement | HTMLLinkElement;

I'm OK with this change. If you submit a PR, please update the Plop template in scripts/plop/templates/component/component.hbs so future components will have the same styles.

But one heads up:

I would like to create some components of my own on top of the Shoelace components.

You might want to read the discussion in #705, specifically this comment.

That said, one problem I see with this proposal is that Shoelace isn't setup for dynamic tags names, which you've touched on above. There are a number of tags hard-coded in stylesheets, templates, and logic. For example, menus require menu items and tab groups require tabs/tab panels. (This is definitely not an exhaustive list.)

By changing tag names, many components will break some will not be obvious at first. Button, for example, relies on <sl-spinner> for its loading state, so if spinner isn't available or gets renamed, you'll have an unregistered/unstyled component in the DOM.

At this time, I don't recommend renaming Shoelace tags or extending them in this way.

[sonntag-philipp]

Thank you for the hint, it was really helpful!
I always try to be as less invasive as possible on the base library when it's possible. My current approach to keep the base working is by simply not touching it at all.
This would also allow Clients who use my component to also use the Shoelace base component directly. That's not really optimal, but after comparing that scenario to other scenarios where my component base would break in very obscure ways due to renamings, I think I can live with applications being able to use the Shoelace base.

Also, the PR is created and ready for review 👍

[claviska]

Fixed in #829. Thanks for the PR!