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

Consider implementing toString() for CSSResult #487

Closed
tlouisse opened this issue Jan 28, 2019 · 12 comments
Closed

Consider implementing toString() for CSSResult #487

tlouisse opened this issue Jan 28, 2019 · 12 comments

Comments

@tlouisse
Copy link
Contributor

tlouisse commented Jan 28, 2019

Description

Currently, CSSResult seems to be designed for LitElements having shadow dom. CSSResult modules tend to be quite closely coupled to the get static styles array as well.

Imagine however the following situation: I have an implementation of a Design System consisting of css modules (meaning equivalents of BEM/SMACCS etc. type of components) that should be reusable in many 'technology contexts' (meaning with and without shadow DOM and in multiple frameworks (React, Angular, Vue, vanilla, whatever).

This example illustrates how one would be able to reuse a 'technology agnostic' implementation of a Design System across different technologies:

For me, it would increase the Developer Experience when I can think about those CSS Modules as being technology agnostic strings.
So then instead of {myModule.cssText}, I could do {myModule}.

Seems like a small thing, but for me it would just be a better experience and it would feel like my styles would be less tightly coupled to CSSResult (and thus LitElement and shadow dom).

Also, It will help decrease verbosity in situations like this:

.grid-container {
  margin: auto;
  max-width: ${unsafeCss(Number(small.cssText.slice(0,-2)) -(Number(containerPadding.cssText.slice(0,-2) * 2)))}px;
}

https://stackblitz.com/edit/css-variables-for-computations?file=style%2Fmodules%2Fgrid-css-module_numbersNotAllowed.js

Would it be possible to add a toString() method that returns the cssText?

Live Demo

See links above.

@sorvell
Copy link
Member

sorvell commented Jan 30, 2019

Would it be possible to add a toString() method that returns the cssText?

This seems reasonable and we welcome a PR for this.

@tlouisse
Copy link
Contributor Author

Will do. Thanks a lot.

@justinfagnani
Copy link
Contributor

I'm not so sure. How is .toString() better than .cssText?

@justinfagnani
Copy link
Contributor

This example will be longer:

.grid-container {
  margin: auto;
  max-width: ${unsafeCss(Number(small.cssText.slice(0,-2)) -(Number(containerPadding.cssText.slice(0,-2) * 2)))}px;
}
.grid-container {
  margin: auto;
  max-width: ${unsafeCss(Number(small.toString().slice(0,-2)) -(Number(containerPadding.toString().slice(0,-2) * 2)))}px;
}

@daKmoR
Copy link
Contributor

daKmoR commented Feb 2, 2019

this is more about interoperability - as we want to use the styling also in other places outside of lit-element.

e.g. see this example without toString...

// colors.js
export const blue = css`#5851ff`;

// render-demo.js
import { blue, headingColor } from 'my-styling/colors.js';

render(html`
  <style>
    div { background: ${blue.cssText}; }
    h1 { color: ${headingColor.cssText}; }
  </style>
  <div>I am the correct blue</div>
`, renderTarget);

and it get's worse the more variables you use.

a sort of workaround...

render(html`
  <style>
    ${css`
      div { background: ${blue}; }
      h1 { color: ${headingColor}; }
    `.cssText}
  </style>
  <div>I am the correct blue</div>
`, renderTarget);

while with support for toString

render(html`
  <style>
    div { background: ${blue}; }
    h1 { color: ${headingColor}; }
  </style>
  <div>I am the correct blue</div>
`, renderTarget);

@tlouisse
Copy link
Contributor Author

tlouisse commented Feb 2, 2019

Thanks for the clarification @daKmoR :)
We mainly use examples like the above in demos in which we want to reuse CSS components (think of a BEM module for a button for instance) outside of our web components.

The goal of toString() would be to make use of type coercion, for a better developer experience.
So with coercion the example would become:

.grid-container {
  margin: auto;
  max-width: ${unsafeCss(Number(`${small}`.slice(0,-2)) -(Number(`${containerPadding}`.slice(0,-2) * 2)))}px;
}

A small difference, but just a bit nicer experience for me.

@justinfagnani
Copy link
Contributor

justinfagnani commented Feb 4, 2019

@tlouisse

Things like this are strictly worse than referencing the cssText property:

`${small}`.slice(0, -2)

Here you're causing a type check on small, a check for the existence of toString, calling toString, generating a new string with the template literal, then calling slice.

This would be much better for the VM (and I'd argue for readers of the code too):

small.cssText.slice(0, -2)

@justinfagnani
Copy link
Contributor

@daKmoR

This doesn't work:

export const blue = css`#5851ff`;

Because #5851ff is not a valid styleSheet.

Maybe what would be better is toString() on cssLiteral?

@daKmoR
Copy link
Contributor

daKmoR commented Feb 4, 2019

@justinfagnani that sure works?

import { grey } from './values.js';

const blue = css`blue`;

class CustomGreeting extends LitElement {
  static get styles() {
    return css`
      p {
        color: ${blue};
      }
      p.grey {
        color: ${grey};
      }
    `;
  }

  render() {
    return html`
      <p>Hello I am blue!</p>
      <p class="grey">Hello I am grey!</p>
    `;
  }
}

https://stackblitz.com/edit/cssvalues?file=custom-greeting.js

and I hope you don't intend to take that away as that is currently the only way to define usable "css constants"... and we use that a LOT.

@justinfagnani
Copy link
Contributor

@daKmoR It really shouldn't work, you should have to use cssLiteral for that. I'll look into this, but we might have to take it away because this wouldn't work if css only generated a StyleSheet object, which is where we want to get to eventually. Why aren't you using cssLiteral?

cc @sorvell

@daKmoR
Copy link
Contributor

daKmoR commented Feb 4, 2019

@justinfagnani cssLiteral got removed here 56bfa0b

if there is a "partial css" then sure we could use that instead - as long as we have the possibility to provide a static constant that can be simply used within a css block that would be fine.

@justinfagnani
Copy link
Contributor

Well, oops. cssLiteral was removed. I see the need now. I would prefer that css is intended for valid CSS always, not fragments which wouldn't parse.

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

No branches or pull requests

4 participants