blakeembrey / free-style

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

Support commas in nested rules #69

Closed pedro-pedrosa closed 6 years ago

pedro-pedrosa commented 6 years ago

When trying to extend third-party CSS rules using free-style you may run into a scenario like this:

const style = FreeStyle.create();
const className = style.registerStyle({
    '& .third-party-input, & .third-party-button': {
        '&:hover': {
            backgroundColor: 'transparent'
        },
        marginLeft: 10
    }
});

This, however, does not work as expected for the :hover rule. The emitted CSS looks something like this:

.f1si452i .third-party-input, .f1si452i .third-party-button {
    margin-left:10px
}
.f1si452i .third-party-input, .f1si452i .third-party-button:hover {
    background-color:transparent
}

Note the missing :hover after .third-party-input in the second rule. This works fine in other CSS preprocessors though (less, sass) so I believe it would be a nice addition to this library replacing the current string replacement logic. Probably just splitting the parent selector and mapping the string replacement function to each element of the splitted selector would suffice here?

blakeembrey commented 6 years ago

This probably won’t be implement because of the technical complexity involved. Parsing a CSS selector is a bit more complicated than splitting on a comma, commas could be inside CSS strings for instance. I do think there’s an easy utility function for this though, since it’s just putting the same object under two different keys.

blakeembrey commented 6 years ago

Would that make sense for you if I added it to a third party utility like https://github.com/blakeembrey/style-helper? I’ve just conveniently avoided parsing any CSS up until now and wouldn’t would to increase code complexity or code size when an approach exists in JavaScript.

pedro-pedrosa commented 6 years ago

Well we always have this

const innerStyle = {
    '&:hover': {
        backgroundColor: 'transparent'
    },
    marginLeft: 10
};
style.registerStyle({
    '& .third-party-input': innerStyle,
    '& .third-party-button': innerStyle
});

which works and avoids code duplication but idk... doesn't feel right.

I'm not sure what you mean by commas being inside css strings? do you mean like div[attr="weird,value"]?

Not sure some clever regex splitting pattern can work here.

Parsing css selectors properly fully is probably going to add complexity. If this is the only use case for parsing css selectors then it's probably not worth implementing.

blakeembrey commented 6 years ago

@pedro-pedrosa Sorry, was on mobile earlier but that's basically the idea and supported today. It's not perfect but I'd prefer not to introduce actually understanding the CSS into the codebase (been lucky so far that I don't need to parse anything). My only other idea was to create a utility which basically does the above for you, e.g.

style.registerStyle({
  ...objectify(['& .third-party-input', '& .third-party-button'], {
    marginLeft: 10
  })
})

In the future we can probably revisit supporting some sort of array/tuple syntax for this, but I think it'd end up even less elegant unfortunately. Something like:

style.registerStyle([
  [['& .third-party-input', '& .third-party-button'], { marginLeft: 10 }]
])
pedro-pedrosa commented 6 years ago

Yeah not ideal.

There are some css parsers out there which parse css selectors into an object model but their size is as big as this module so it's probably not worth it for now.

As for the utility function, it could be useful yeah (at least it doesn't make you store the inner style in a variable or create an immediately called lambda). Although I'd call it multiAssign or something I guess.

blakeembrey commented 6 years ago

That's fair, it's just already in style-helper as objectify (previously it just didn't support array keys, I just pushed that change https://github.com/blakeembrey/style-helper/commit/ba9d329857685058632e4efaf847ecc101546cc7).