Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support commas in nested rules #69

Closed
pedro-pedrosa opened this issue Mar 22, 2018 · 6 comments
Closed

Support commas in nested rules #69

pedro-pedrosa opened this issue Mar 22, 2018 · 6 comments
Labels

Comments

@pedro-pedrosa
Copy link

pedro-pedrosa commented Mar 22, 2018

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

blakeembrey commented Mar 22, 2018

@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
Copy link
Author

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
Copy link
Owner

blakeembrey commented Mar 22, 2018

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 blakeembrey/style-helper@ba9d329).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants