bss icon indicating copy to clipboard operation
bss copied to clipboard

Allow `style` and `class` output to be determined early on in the chain

Open barneycarroll opened this issue 6 years ago • 20 comments

There's something I really dislike about assigning BSS as style and class (and .toString() TBH) at the trailing end of a multi-line, multi-block chain. Having that stuff visible ahead of time would be good.

What do you think about introducing simple b.class and b.style functions that simply consume an object (or operate as tagged template interfaces like b) and return the corresponding property of the same name?

barneycarroll avatar Jun 29 '18 19:06 barneycarroll

I very rarely need either .class or .style due to how .valueOf() gives the className, and I don't really need .style because I'm using spread. example

Would you mind making an example of the issue?

Just to be sure, you mean having something like this right? I suppose they could be exported as named exports? (although the name class won't work).

porsager avatar Jun 29 '18 23:06 porsager

Actually my idea was just b.style = x => x.style. But - I didn't realise only style was enumerated during spread, which is really neat! I need to step away from the code base for a bit to get some distance. Did I really need the class/Name property? Probably not…

barneycarroll avatar Jul 02 '18 17:07 barneycarroll

Hey @barneycarroll . Do you want to elaborate on this, or is the option of using spread a good enough solution for you?

porsager avatar Jul 15 '18 18:07 porsager

Yeah I can do this in userland. Unshakeable thought tough - I'm wondering if we could somehow overload BSS with an iterator API to expose class. So m(X, ...b()) yields a class, m(X, {...b()}) yields style? Hmm.

barneycarroll avatar Jul 15 '18 23:07 barneycarroll

Wow.. that would be some pretty cool overloading 😀

I'm still not sure what m(X, ...b()) brings instead of m(X + b())?

Ahh, I think I might get it now... Is it for components where you want to pass attributes to the inside with the class? Sorry if I'm just repeating what you stated, I just want to be sure I get it right ;)

So m('div' + b()) is all good, but m(component + b()) of course won't work, and m(component, { class: b().class }) seems like it could be better?

Another thing that could be doable is having b.class and b.style return template functions on a raw instance which returned an object with either { class } or { style } so you could do as requested like:

m(component, b.class`
  background red
`)

and

m(component, {
  special: 'wat',
  ...b.style`
     background black
  `
})

I'm reopening again since I don't want to loose the awesomeness if you're on to something 😁

porsager avatar Jul 16 '18 22:07 porsager

Yes, that last thing was my original thought. The class hack is a bit silly, I realise. The point is, when composing many parts of a whole, with conditional classes etc, with interpolations, along with other conditional aspects to a vnode expression… That in these scenarios relying on a dangling property makes the whole expression that significant bit harder to read. Eg

m('whatever' + b`
  some stuff
`
  .$nest('etc', `
    some more
  `)
    .class // essential to the comprehension of prior X lines / expressions
)

barneycarroll avatar Jul 16 '18 22:07 barneycarroll

I see 😊

Sorry if I'm being pedantic, but in your last example, .class would not be needed?

It would have to be something like

m(component, {
  class: b`
    .....
  `.class
})

right?

porsager avatar Jul 16 '18 22:07 porsager

FFS I need to sleep, my brain is muck!

barneycarroll avatar Jul 16 '18 22:07 barneycarroll

Maybe it's better to export multiple b that user can select from?

Like patchinko that have P, O for different purpose.

So like below code:

import b, {bc} from 'bss'

<div className={bc.m(10)}>

// same as below:
<div className={b.m(10).class}>

Is that possible/resonable?

The class thing is annoy when using JSX, remove .class tail is good for React world.

futurist avatar Jul 26 '18 12:07 futurist

I think I love that idea @futurist, not sure that's what barney meant, but also very relevant!

What you mention wrt. react, is possible to do now, but I'm not happy with the solution mentioned in the readme about overriding valueOf either.

porsager avatar Jul 28 '18 19:07 porsager

Wrt. React, something like this could also be done, which I think is pretty cool

<h1 {...b`c red`}>Hello React!</h1>

Ok, so taking these thoughts further brings @barneycarroll 's original thought into it again I think..

Bss exports 3 things b(same as default), c for class, s for style..

b works as we know it c will only have className as enumerable property (allows for jsx example above), and valueOf will return the className without a period (which b does) s will have every property specified enumerable to use with style / dom element assignment

porsager avatar Jul 28 '18 19:07 porsager

it's very cool! Then it could possible do below thing in React with classnames:

<div className={{opened:true,  [c`c red`]: true}}></div>

The concern is to use with user passed classes like opened, only way I can think about is use classnames module like above, but it's totally fine I think.

futurist avatar Jul 28 '18 23:07 futurist

Or the c can have the below forms? (c.class method)

<div {...c.class('opened nav').`c red`}></div>

<div {...c.class({opened: true, nav: false}).`c red`}></div>

futurist avatar Jul 31 '18 06:07 futurist

I'm considering, amongst other breaking changes in version 2, changing BSS instances to work like this:

  • bss will keep valueOf as returning '.' + className.
  • style property will be removed as enumerable
  • class property will be removed
  • className will be added as the only enumerable property such that the examples below will be valid.
// JSX
<h1 {...b`c red`}>Hello React!</h1>
// Hyperscript
m('h1' + b`c red`, 'Hello Mithril!')

// or
m('h1', b`c red`, 'Hello Mithril!')

// or
m('h1', {
  title: 'Yeah this is not obvious',
  ...b`c red`
}, 'Hello Mithril!')

// Components - if the component freely passes attributes to an element inside.
// I think this is a really neat way to make components styleable.
m(SomeComponent, {
  ...b`c blue`,
  customAttr: 'wat'
})

porsager avatar Aug 02 '18 10:08 porsager

@futurist I really like your examples too!! Not sure I'd want to mix foreign classnames into bss, but I'll try to think about it..

porsager avatar Aug 02 '18 10:08 porsager

If you have time @barneycarroll I'd also love to hear your thoughts on the changes above... Thanks

porsager avatar Aug 02 '18 12:08 porsager

The spread operator will overwrite any existing className attr, in v2 I think this should be taking into account.

futurist avatar Aug 05 '18 02:08 futurist

@futurist yeah, that's a very good point.. I was actually a victim to it myself just yesterday.

The reason why I'd rather want className than style is that style won't allow pseudo classes to be set, so using style for this kind of composition limits the options.

One way of solving it could be adding whitespace around the returned className.

Then you'd make a stylable component like this:

const Component = {
  view: ({ attrs, children }) =>
    m('div'
    + b... // General styling
    + attrs.className // Component consumer styling
    + b... // Styling not overridden by consumer styling
    ,
      children
    )
}

porsager avatar Aug 06 '18 07:08 porsager

As discussed/discovered last year on gitter, it appears we can have the best of both world by having valueOf return ' . ' + this.class + ' ' which makes it work with hyperscript text concat and attrs class/className. I'll be experimenting a bit more with this for v2.

porsager avatar Feb 16 '19 13:02 porsager

@barneycarroll I'm planning to land the ' . ' + this.class + ' ' solution in v2, do you think there's anything more to add to the concerns in this issue?

porsager avatar May 21 '19 21:05 porsager