porsager / bss

🎨 Better Style Sheets
Do What The F*ck You Want To Public License
91 stars 14 forks source link

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

Open barneycarroll opened 6 years ago

barneycarroll commented 6 years ago

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?

porsager commented 6 years ago

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).

barneycarroll commented 6 years ago

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…

porsager commented 6 years ago

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

barneycarroll commented 6 years ago

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.

porsager commented 6 years ago

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 😁

barneycarroll commented 6 years ago

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
)
porsager commented 6 years ago

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?

barneycarroll commented 6 years ago

FFS I need to sleep, my brain is muck!

futurist commented 6 years ago

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.

porsager commented 6 years ago

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 commented 6 years ago

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

futurist commented 6 years ago

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 commented 6 years ago

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>
porsager commented 6 years ago

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

// 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 commented 6 years ago

@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 commented 6 years ago

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

futurist commented 6 years ago

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

porsager commented 6 years ago

@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 commented 5 years ago

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 commented 5 years ago

@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?