hybridsjs / hybrids

Extraordinary JavaScript UI framework with unique declarative and functional architecture
https://hybrids.js.org
MIT License
3.03k stars 85 forks source link

Class attribute mix-in _on_ the web-component #192

Closed gotjoshua closed 2 years ago

gotjoshua commented 2 years ago

Not sure if its actually an anti-pattern, but i like to put classes onto the root web-component element itself. I found this to be not so seamless DX.

I made a sandbox forking the simple counter example

i got it working via render or content function that includes these lines:

 render: (host) => {
    const { count, className } = host;
    const classDefaults = "static-string works-fine";
    host.classList.add(...classDefaults.split(" "), ...className.split(" "));

results:

<simple-counter class="from-instance static-string works-fine" count="0">

All fine (as a work around), but i expected this to work:

{
    class: ({ className }) => `does-not-work ${className}`, 
    // this doesn't work as it defines a getter that is apparently not used for rendering
}

I understand from the docs that class and style are treated specially, but I am not sure if the lack of mix-in functionality is a feature to be expected, or a bug that needs attention.

gotjoshua commented 2 years ago

Kinda related to https://github.com/hybridsjs/hybrids/issues/95 but different due to the wish for composable classes on the root level of the component.

smalluban commented 2 years ago

I'll need to understand your use case for adding classes to the host element to be able to help you. If you use render function, you create a Shadow DOM, so you can simply add :host {} CSS rules to style your host element, like this:

render: () => html`...`.css`:host { color: red }`

If you put classes on top of your host element, the styles must be defined in the upper scope, so if your element will be used in another Shadow DOM (in the render method of another web component) global styles (put in document level) will not work.

In that sense, your way is an anti-pattern, as web components should style themself. Of course if you want to overwrite the styling, you can do it just like in HTML in old days:

render: () => html`
  <simple-counter class="from-instance static-string works-fine"></simple-counter>
`.css`
  .from-instance { ... }
  ...
`
gourmetseasoningsake commented 2 years ago

@gotjoshua If I may add something to this discussion, I once fiddled with this (or a similar) idea in a tailwind project. As far as I can remember, the following worked for me and if i get you right, it might be a bit simpler than your workaround.

const classDefaults = defaults => ({
  value: defaults,
  connect: host => { host.className = `${defaults} ${host.className}` }
})

export default define({
  class: classDefaults("some default classes")
  // ...
})
gotjoshua commented 2 years ago

Ah, I forgot to mention i want to add classes that are defined in Unocss (its a Tailwind / Windicss inspired lib). So it is important that the component consumers can add additional css class rules to the component root. And I don't want a nested div, if not needed.

Here is my full example repo: https://gitlab.com/onezoomin/edge-templates/parcel-edge-component-template/-/blob/trunk/src/flex-row.ts


This looks nice:

render: () => html`
  <simple-counter

but when i tried it (using the tag of the component as the top level), it creates an infinite loop of rendering a new instance nested inside itself. Am I missing something?

const classDefaults = 'h-fit w-fit frow-g2'
const childrenClassAdditions = 'h-fit b-1 p-2'
const FlexRow: Component<FlexRow> = {
  tag: 'flex-row',

  class: '', 
  'classafter': `gap-2`, // be aware trumping may not work as you expect it to (based on the order of rule _definition_ NOT application order)

  render: (host: typeof FlexRow) => {
    const { children, classafter = '' } = host

    // host.classList.add(...classDefaults.split(' '), ...classafter.split(' ')) 

    return html`
<flex-row class="${classDefaults} ${classafter}">
  ${[...children].map(item => {
    item.classList.add(...childrenClassAdditions.split(' ')) 
    console.log({item, cn: item.className})
    return item
  })}
</flex-row>
`
  },
}
gotjoshua commented 2 years ago
connect: host => { host.className = `${defaults} ${host.className}` }

ooo thanks @gourmetseasoningsake !
what is this fancy connect prop?? will try it out...

gourmetseasoningsake commented 2 years ago

@gotjoshua it's this thingy here

Also, although I'm making some assumptions here, this is roughly how I would approach it. It may well depend on how you apply your global styles.

// someHelperFile.js

const classDefaults = defaults => ({
  value: defaults,
  connect: host => { host.className = `${defaults} ${host.className}` }
})

// FlexCol.js
// imports classDefaults

export default define({
  tag: "flex-col",
  class: classDefaults("some default classes"),
  // ...
  render: () => html`<slot></slot>`
})

// FlexRow.js
// imports classDefaults

export default define({
  tag: "flex-row",
  class: classDefaults("some default classes"),
  // ...
  render: () => html`<slot></slot>`
})

// someEntryFile.js
// imports flex-row/col

define({
  tag: "some-wrapper-tag",
  content: () => html`
    <flex-row class="some additional classes">
      <flex-col class="some additional child classes">...</flex-col>
      <flex-col class="some additional child classes">...</flex-col>
      <flex-col class="some additional child classes">...</flex-col>
    </flex-row>
  `
})
gotjoshua commented 2 years ago

Super clear example @gourmetseasoningsake , many thanks for taking time to engage!

connect: host =>

By what js voodoo does that helper function get access to the host!?!

And will it work reliably in situations with nested components?

And will it work well if classes are added reactively?

Super curious to try it out tomorrow!!

gourmetseasoningsake commented 2 years ago

By what js voodoo does that helper function get access to the host!?!

From the docs property descriptor and factory functions might be of interest to you.

And will it work reliably in situations with nested components? And will it work well if classes are added reactively?

I'd say yes :) Here is a simple example: https://codepen.io/mohammed_lee/pen/PoRNPqQ Hope it helps.

gotjoshua commented 2 years ago

Small update from my side: Current experiments are here: https://gitlab.com/onezoomin/edge-deploy/nested-data-grid

i ended up using a mix of @gourmetseasoningsake suggestions and my workarounds. I discovered that using string based className concat led to duplicates. So i went with classList.add, but implemented it in the connect prop: https://gitlab.com/onezoomin/edge-deploy/nested-data-grid/-/blob/trunk/src/components/flex-col.ts#L12


const classDefaultsConnector = (defaultClasses: string) => ({
  value: defaultClasses,
  connect: host => host.classList.add('|defaults|',...defaultClasses.split(' ')), 
})

results in defaults clearly visible after the '|defaults|' divider

<flex-col class="gap-2 grow min-w-1/2 p-0 |defaults| fcol-g2">

thanks again @gourmetseasoningsake (also just found your starter repo template now - could've saved quite a bit of time if i found it earlier i guess)

I'd still love some more feedback from the core dev(s) about best practices for tailwind style styling...

smalluban commented 2 years ago

I'd still love some more feedback from the core dev(s) about best practices for tailwind style styling...

Most CSS frameworks are built on one ground rule - the result CSS is attached on the document level. That rule can become an issue quickly if you build custom elements with Shadow DOM. Then, the root element is a shadow root rather than a document.

How you want to deal with it depends on the case. If you build a SPA-like application, and you have full control over how you use elements, you may choose to not use Shadow DOM at all (in hybrids, by using content property instead of render). Still, As the hybrids depends on the native DOM distribution (so it does not provide its own implementation), then you might be hit if you suddenly need it (I mean using slots elements).

I think it might be your case here. However, if you build elements, which are then used by others, they might be put in another Shadow DOM by the user (for example, by putting them in another web component shadowRoot). In that case, the best way is to avoid global styling, and style the elements inside of the Shadow DOM.

Personally, I prefer to build "views" using content without any CSS and "visual" components (sometimes called dump, or leafs), and there use Shadow DOM and internal styling. At the same time, I see, that it might become very verbose, to create another web component, if I have a single case with layout or something else (that's why the tailwind or another atomic CSS framework came into the world), so I am currently designing the internal layout system similar to the tailwind concept, which will allow styling "views" and "visual" components quicker, without need to create CSS by hand.

gourmetseasoningsake commented 2 years ago

I discovered that using string based className concat led to duplicates.

You're right, i think you would need to set the value prop to an empty string.

smalluban commented 2 years ago

@gotjoshua Feel free to re-open if you need more help in the subject. By the way, you may want to see #194. I am working on a built-in layout system similar to tailwind and other atomic CSS frameworks.