blakeembrey / free-style

Make CSS easier and more maintainable by using JavaScript
MIT License
707 stars 29 forks source link

API for 'adding' to generated class? #52

Closed devdoomari closed 7 years ago

devdoomari commented 7 years ago

is it possible to 'add' more to a generated classname?

e.g)

const someClass1Name = registerStyle({ ... });
...
freeStyle.addMoreTo( someClass1Name, {
    '@media for iphone5 ', { ... }
);
freeStyle.addMoreTo( someClass1Name, {
    '@media for ipad ', { ... }
);

(also, I need to have the order preserved...)

blakeembrey commented 7 years ago

No. If you want this, you're probably using it wrong. If this existed, it would break the fundamental underlying property that every style is a hash of it's content. Could you explain what you want to achieve with this?

devdoomari commented 7 years ago

so

className1 = registerStyle( styleData ) 

returns:

className1 = someHash( styleData )

???

devdoomari commented 7 years ago

Then, how about extending registerStyle to have more-than-one inputs? or 'registerStyles'?


className1 = registerStyles([
     styleData1,
     styleData1ForIphone,
     styleData1ForIpad,
]);
blakeembrey commented 7 years ago

Yes, the class name is computed from style, but I still don't understand what you'd like to see - can you give a concrete example?

devdoomari commented 7 years ago

it's all about having the order preserved for media queries... So, for a bit more concrete example:

freeStyle.registerStyles([
  {
     width: 500
  }, {
      '@media for wider-than-iphone5 ': { width: 700 }
  ), {
      '@media for wider than ipad': {width: 800, padding-right: 0}
  }
]);
blakeembrey commented 7 years ago

Ok, but that already occurs with the object form - do you have a concern that it's not working? It might be possible to make this change, so I'd welcome a PR. Do you have an idea of what we'd make it look like in the nested form?

blakeembrey commented 7 years ago

Perhaps we go for a "tuple" style using an array of arrays where the the first item is the key and second is the style object? Gets tricky with the current approach though, because you insert the style object initially so there'd be no key.

devdoomari commented 7 years ago

but that already occurs with the object form

nope. the current object form can't preserve ordering - it's dictionary after all.

Perhaps we go for a "tuple" style using an array of arrays where the the first item is the key and second is the style object

That would be a real over-kill... of course you can do that by applying lodash function-dabs, but it's not about height-comes-first-or-width-comes-first. Those do not affect the rendered result. But ordering of CSS media queries DOES affect the end result.

blakeembrey commented 7 years ago

@devdoomari Yes, it can. Have you even tested it? Please don't waste time arguing about something you can just check in a console. All browser engines have ordered by the way you wrote the object for a long time, and IIRC it's even formalised recently. For example:

image

That would be a real over-kill...

What are you talking about? I'm trying to help come up with a proposal like yours that works. I'm trying to work with you.

blakeembrey commented 7 years ago

I'm still not sure what you're referring to when you suggest lodash function-dabs or the rest of that comment. If something I said didn't make sense, it would be great to let me know, because I'm really not sure where lodash comes into anything. All I was suggesting was a structure like:

registerStyle([
  { width: 500 },
  ['@media print', { color: red }],
])

The issue is it needs to be unambiguous. This works for a single level, but what about nesting?

registerStyle({
  width: 500,
  li: {
    color: 'red',
    '@media print': {
      color: 'blue'
    }
  }
})

That needs to be supported with any syntax that gets adopted - we need to make sure it can work recursively.

blakeembrey commented 7 years ago

I guess both approaches could use arrays of objects to indicate ordering. Then it comes down to whether we prefer the key to be part of a single-element object or separate as a tuple. This is definitely a viable alternate syntax we can introduce, but whatever proposal it is needs to be solid since it can never be undone. Also note that this may be non-trivial, since arrays today are 100% valid in value positions (https://github.com/blakeembrey/free-style/blob/80253357ba0ce1cba5709b641e8aeca73e0f0cc6/src/free-style.ts#L89). It's currently a non-issue as no browser engine has an issue with keeping the order of object keys (here's an article on it, not sure if it made it to spec: http://2ality.com/2015/10/property-traversal-order-es6.html).

devdoomari commented 7 years ago

so do you plan to do a fix on this? I'm not keen on depending on Objects for preserving order (it's funky on IE... / being on ES6 doesn't guarantee any browser compatibility except for chrome / FF ) -- maybe WeakMap (from ES6, usually with Babel-Polyfill) will do better for browser compatibility, as WeakMap shims are made with keeping order in mind.

Also, if you just remove sort tuples function inside FreeStyle, object-order is somewhat preserved... (* but of course this will break a lot of test cases...)

Here's the test code:

const styleData: FreeStyle.Styles = {};
const style = FreeStyle.create();
styleData['color'] = 'red';
styleData['background-color'] = 'blue';
style.registerStyle(styleData);

const styleData2: FreeStyle.Styles = {};
const style2 = FreeStyle.create();
styleData2['background-color'] = 'blue';
styleData2['color'] = 'red';
style2.registerStyle(styleData2);

const style1str = style.getStyles();
const style2str = style2.getStyles();
console.error('style:', style1str);
console.error('style2:', style2str)
blakeembrey commented 7 years ago

so do you plan to do a fix on this?

What do you expect to be fixed? So far, you haven't told me what is broken. Object order is already preserved. Can you please provide a reproducible case that is broken? Sorting CSS styles is a feature and has no impact on the output, but does make a big difference to hashing. I won't be changing how styles are ordered, and nested objects are already preserved in their correct order (they shouldn't be sorted, you can read the code).

Edit: Aren't -> shouldn't be, since they really shouldn't be but there could be an issue there I haven't encountered. I also agree that a syntax could be adopted to support arrays, we just need to agree on what it should look like and open a new issue about it. This issue was about "adding to a generated class" which seems different to what you're now asking for.