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

Support more variants of css inside lit components #47

Open
iSokrat opened this issue Mar 31, 2023 · 6 comments
Open

Support more variants of css inside lit components #47

iSokrat opened this issue Mar 31, 2023 · 6 comments

Comments

@iSokrat
Copy link

iSokrat commented Mar 31, 2023

Now, the plugin works with css places inside css tag. However, css may often be stored near html

Example 1 (html tag):

import {styles} from 'somewhere';

//...

html`
  ${styles}
  <style>
     .abc {
       color: green;
     }
  </style>
  <h1> ... </h1>
  ....
`

Example 2 (svg tag):

import {styles} from 'somewhere';

//...

svg`
  ${styles}
  <style>
     // ....
  </style>
  <svg> ... </svg>
  ....
`

There are also some minor cases when some styles are inlined:

html`
  <h1 style="color: green"> ... </h1>
`

The proposal is to at least add support of first 2 major cases (Example 1, 2), however, would be nice to support all possible variants.

@iSokrat
Copy link
Author

iSokrat commented Apr 5, 2023

@43081j Hello, what do you think about this?

@43081j
Copy link
Owner

43081j commented Apr 8, 2023

It's a difficult one to solve as it means we need to start parsing html/SVG templates in lit components just in case they contain CSS.

These days I would hope everyone uses stylesheets instead.

Do you have an example of why someone would want inline CSS instead of writing it in a style template?

It's a fair chunk of work so if we can instead just tell people to use CSS templates, it'll save a lot of effort

@iSokrat
Copy link
Author

iSokrat commented Apr 10, 2023

So, I'll try to describe my case:

My project uses hundreds of lit components, some of them were created long time ago, some of them - not.

Do you have an example of why someone would want inline CSS instead of writing it in a style template?
Usually, inline CSS means this:

html`
  <h1 style="color: green"> ... </h1>
`

I fully agree that it's not a good practice. Above I mentioned this case as minor one, it's not really critical - on my project only couple of places have this in some legacy code. In case if it's possible to support such cases without big efforts (we'll try to parse style attribute), then that's cool, if not then it's not a big problem.

About html and svg

So, svg is also in my case mostly an exception since I have only one component fully based on some graphical data.
But html is not. This is widely used practice on my project since it has own benefits:

  1. Often there are not a lot of styles for some components and based on the practice it's easy to store them near html-like code to quickly see the total picture (our render functions are stored usually in the separate files, not in the element's body). https://lit.dev/docs/components/styles/#style-element
  2. From performance perspective I personally didn't see any suspicious things (we don't use expressions inside styles to not have some performance issues).
  3. This approach supports the same scoping of styles inside Shadow DOM.

So, even in case if it would be needed in the future to re-write everything to css, for my project it will not be so simple task.

@iSokrat
Copy link
Author

iSokrat commented Apr 12, 2023

@43081j Hello, what do you think about described above?

@43081j
Copy link
Owner

43081j commented Apr 16, 2023

it would introduce a fair amount of overhead in the syntax parsing to support the <style> tags.

ideally you would migrate to style templates, for the various benefits of adopted stylesheets (e.g. the fact that an adopted stylesheet can be shared across many instances of an element, while style tags will be duplicated).

though i understand such a migration takes a lot of time, if you even have the chance to do it (or want to do it).

we could possibly put <style> tag extraction behind a flag, but it still means this module needs to then pull in parse5 and implement some basic html traversal.

its difficult, as i know it would help you and some others a lot but its an expensive piece of work (time consuming). we'd have to implement html template extraction, using parse5 to parse the templates and meanwhile replacing interpolations with placeholder html. basically what eslint-plugin-lit does under the hood.

its a lot of logic to introduce for a user case people should be moving away from

@iSokrat
Copy link
Author

iSokrat commented Apr 17, 2023

though i understand such a migration takes a lot of time, if you even have the chance to do it (or want to do it).

Yep, this may take some time to do this. We already have own eslint plugin which checks some basic project-related things in <style> created long time ago, so, this was the first reason why I want to use tome other tool used more often for such things.

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

No branches or pull requests

2 participants