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

Default theme should apply to :host as well as :root selector #437

Closed
aomarks opened this issue Apr 26, 2021 · 14 comments
Closed

Default theme should apply to :host as well as :root selector #437

aomarks opened this issue Apr 26, 2021 · 14 comments
Assignees
Labels
bug Things that aren't working right in the library. themes Anything to do with the theming.

Comments

@aomarks
Copy link

aomarks commented Apr 26, 2021

Describe the bug
The default theme stylesheet only applies to the :root HTML element, not the :host for the case where Shoelace is to be encapsulated within another element that uses a shadow root.

To Reproduce
Steps to reproduce the behavior:

  1. Define a custom element
  2. Import a shoelace component
  3. Create a shoelace component in the element's shadow root
  4. Load the default theme CSS <link> in the element's shadow root
  5. Theme is not applied

Here's a link to a repro: https://tinyurl.com/wwcm6wfp

Expected behavior
I should be able to use a Shoelace component within my own distributed component, without requiring the end-user to load the Shoelace stylesheet in the top level of their app.

Suggestion
If the default stylesheet rule used the selector :root, :host instead of simply :root, then this is possible.

@aomarks aomarks added the bug Things that aren't working right in the library. label Apr 26, 2021
@claviska claviska added the themes Anything to do with the theming. label Apr 26, 2021
@claviska
Copy link
Member

Interesting. Are there any known repercussions with scoping design tokens to :host as well as :root?

The original idea was to set them globally on the root so the user can override them anywhere in the cascade. My knee-jerk reaction to this is that scoping to :host would cause problems here, but it looks like we can still target components and even arbitrary parent containers with this approach. 🤔

@aomarks
Copy link
Author

aomarks commented Apr 26, 2021

Interesting. Are there any known repercussions with scoping design tokens to :host as well as :root?

The original idea was to set them globally on the root so the user can override them anywhere in the cascade. My knee-jerk reaction to this is that scoping to :host would cause problems here, but it looks like we can still target components and even arbitrary parent containers with this approach.

No negative repercussions I can think of. With custom properties, outer scopes have precedence over inner scopes (spec), so you'll always be able to override any internal definitions from the outside, including at the top-level document scope. (edit: sorry, this part was incorrect)

MWC for example loads its default styles directly into the component definition. So it has a reasonable default style out-of-the-box, but users can always override any custom properties it uses from the outside.

https://unpkg.com/browse/@material/mwc-button@0.20.0/mwc-button.js
https://unpkg.com/browse/@material/mwc-button@0.20.0/styles-css.js

@claviska
Copy link
Member

So to recap, adding the :host selector will let us style Shoelace components that are used inside shadow roots whether or not the stylesheet has been imported globally. This makes sense, but I have one concern.

Consider this example

I've added the global stylesheet to index.html along with :host/:root scoped styles in the shadow DOM. You'll notice that the --sl-color-primary-500 token has been updated as follows:

--sl-color-primary-500: var(--sl-color-primary-500, tomato);

The idea is to use the root design token if one is specified and fallback to tomato if one is not. Ideally the button would be tomato colored, but in this case, the :root selector in the shadow DOM doesn't allow it to fallback as we might expect.

My concern is that having both scopes means there's no way to fallback to a token higher up in the cascade. 😕

It seems like we need a set of custom properties that can be imported and scoped to either :root, :host, or possibly an arbitrary selector. One way I can see this happening is a JavaScript object with a utility method that accepts a user-provided selector. Something along the lines of:

const tokens = { ... };

export function getBaseTokens(selector = ':root') {
  return css`
    ${selector} {
      ${stringify(tokens)}
    }
  `;
}

Then to use it in a third-party component:

import { getBaseTokens } from '@shoelace-style/dist/utilities/tokens';

@customElement('my-component')
class MyComponent extends LitElement {
  static styles = [getBaseTokens(':host'), ...];
}

As an aside, I'd like to document the design tokens better so I can add them to metadata.json and ultimately the docs. If we're going down this path, I'd probably add JSDoc comments to describe each token while I'm at it.

I think this would also solve #438, but I'd like to hear your thoughts on this approach.

@aomarks
Copy link
Author

aomarks commented Apr 26, 2021

The idea is to use the root design token if one is specified and fallback to tomato if one is not. Ideally the button would be tomato colored, but in this case, the :root selector in the shadow DOM doesn't allow it to fallback as we might expect.

🤦 Sorry, I just realized one of my earlier comments was wrong -- I was incorrectly remembering that you'd always be able to override a property assignment from an outer scope, but this isn't true. So yeah, you're right, you can't simply load the default property values and apply it with :host, and still allow outer scopes to override those values.

Maybe this is overly simplistic, but if you baked the default theme values directly into the var(...) consumption points, instead of having no fallbacks with defaults provided through a separate stylesheet, then would that allow sufficient control, while providing a good out-of-the-box experience? (You could use SASS variables to manage the defaults so that they still have a single source of truth).

Here's an example which also shows distributing a dark theme module, which can be applied with an import.

@claviska
Copy link
Member

I've played with the fallback method before and it gets a bit unwieldy to maintain styles that way. I've always assumed the base stylesheet to be a root-level dependency, otherwise there's no clear way to share design tokens amongst different components and across shadow roots.

There are also a couple utility styles such as sl-scroll-lock and a [hidden] override that some components rely on. These could arguably be handled differently, but it's a good example of the occasional need for a base stylesheet with root-level selectors.

However, I do appreciate the use case you're presenting. I'd like to expose design tokens in an importable way, but I'm not sure how to do that without making them JavaScript-first, e.g.:

export default {
  '--sl-color-black': '#000',
  '--sl-color-white': '#fff',
  ...
};

This may be helpful to component devs, but not so much for consumers who want to be able to use the cascade to override a handful of design tokens in CSS. (The JS-based tokens would be out of sync as soon as any override occurs.)

Going back to your suggestion of using CSSResult, maybe the export would look like this:

export default function getTokens(selector = ':root') {
  return css`
     ${selector} {
      --sl-color-black: #000;
      --sl-color-white: #fff;
      ...
    }
  `;
}

This would allow you to grab the full set of design tokens with a custom selector (e.g. :root or :host). But again, changes made at the root wouldn't be picked up. Each call to this function will be a hard reset of the theme. That may be what we want for this use case, but I wonder if it will cause confusion when others inevitably import the theme this way and expect it to work a different way.

I'm still not sure what the correct path forward is for this. Feedback is always welcome!

@aomarks
Copy link
Author

aomarks commented May 11, 2021

I've played with the fallback method before and it gets a bit unwieldy to maintain styles that way.

Do you mean it's unwieldy to manage the defaults given that you need to consistently replicate the same fallback value across multiple var statements? Since you're already using SASS, could you use SASS variables in the fallback positions so that you still have a single source of truth?

I wonder if there's a downside to this approach other than the extra byte cost?

There are also a couple utility styles such as sl-scroll-lock and a [hidden] override that some components rely on. These could arguably be handled differently, but it's a good example of the occasional need for a base stylesheet with root-level selectors.

Maybe these could be factored out into modules that export a CSSResult, and then you could import and include them in the styles array in the components that need them.

Isn't this pattern of using attribute/class selectors already a bit of an issue -- because even if you include them at the root, they won't apply through shadow roots, so you can't really do composition anyway?

@claviska
Copy link
Member

claviska commented May 12, 2021

Do you mean it's unwieldy to manage the defaults given that you need to consistently replicate the same fallback value across multiple var statements? Since you're already using SASS, could you use SASS variables in the fallback positions so that you still have a single source of truth?

Exactly. On top of the verbosity, I'm reluctant to use Sass variables because I don't currently use them anywhere in the source and I was hoping to move to PostCSS soon. (Currently, Sass is only used for nesting and importing.)

Maybe these could be factored out into modules that export a CSSResult, and then you could import and include them in the styles array in the components that need them.

The sl-scroll-lock class is always applied to the body when, for example, a dialog is open. Arguably, this could be done by setting the appropriate styles directly on the body, but then we have to worry about overriding user-defined body styles and restoring them later (not to mention what happens if body styles change while scrolling is locked — we might restore the wrong value).

The other is a [hidden] override that makes the specificity of the hidden attribute much higher. This probably isn't the best approach, though, so I'll work to remove that one.

Edit: the base theme also includes .sl-toast-stack styles that are used to support the alert's toast() feature.

@claviska
Copy link
Member

The [hidden] styles have been removed in cae5086. That really shouldn't have been there in the first place.

@aomarks
Copy link
Author

aomarks commented May 12, 2021

Exactly. On top of the verbosity, I'm reluctant to use Sass variables because I don't currently use them anywhere in the source and I was hoping to move to PostCSS soon. (Currently, Sass is only used for nesting and importing.)

Ah I see. Have you considered an extra set of variables to represent the defaults?

E.g.

color: var(--my-property, var(--my-property-default));

The default theme stylesheet could then set the -default versions of those properties instead. That would seem to solve the problem of not being able to override from a higher scope. A component that's encapsulating a shoelace component could then include these default styles in its own shadow root, but an end-user could still set --my-property directly if they want to override the default. (Note I'd expect adding these fallback values would have a pretty minimal effect on compressed size, since it would be such a regular pattern.)

Or another option, if you're switching to PostCSS, might be to do the same as above, but then transform the -default properties into their concrete values at build time using https://github.com/MadLittleMods/postcss-css-variables.

@claviska
Copy link
Member

claviska commented May 13, 2021

Good suggestion. I'm not too worried about the size of the fallbacks, but more the inconvenience of having to write them out everywhere and the opportunity for human error that will likely result. It may be a necessary evil, though. 😈

I'm still not sure exactly how to solve the theming problem. Initially, I was hoping to rely on "normal CSS" for customization since this is especially popular with many users. On the other hand, this requires that the ancestral root (e.g. document or shadow) include the design tokens.

This is sort of converging with #438, particularly this comment. Do you have any thoughts on that?

I appreciate your insight. Theming is the next major thing I want to tackle, and I want to get it right for both consumers and components devs — just not sure how to do it with minimal friction for both.

@e111077
Copy link

e111077 commented May 27, 2021

Heya, I ran into this not applying to host too. I was trying to do the following pattern:

// using rollup-plugin-postcss-lit to import the styles when I need them
// and to help fragment
import shoelaceStyles from '@shoelace-style/shoelace/dist/themes/base.css';
import styles from './my-view-3.scss';

@customElement('my-view-3')
class MyView3 extends LitElement {
  static styles = [shoelaceStyles, styles];
  ...
}

Thing is to do this, I turn off postcss injection

Personally I'd prefer not to have to have a so not to have to deal with copying files over and keeping everything within rollup.

Luckily the base styles file is so small, I was able to just ctrl+c ctrl+v

@claviska
Copy link
Member

Yep...I'm still working through a backlog of smaller things before tackling theming. Probably going to ditch Sass and start exporting CSS and design tokens as mentioned in #438, that way they can be injected into arbitrary shadow roots. We also need a way to facilitate customizations, so any thoughts or opinions on that before I start experimenting are appreciated!

@claviska
Copy link
Member

Closing this so we can focus on it more actively in #438. Comment to follow.

@claviska
Copy link
Member

The changes requested here were applied in beta.48 which was released today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Things that aren't working right in the library. themes Anything to do with the theming.
Projects
None yet
Development

No branches or pull requests

3 participants