-
Notifications
You must be signed in to change notification settings - Fork 815
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
Allow lists of nested selectors and allow styles after nested CSS blocks #4040
Conversation
@rodrigogiraoserrao Please add some detail on what you fixed (and how). The issues you linked to don't make it clear what work is outstanding. The title is also rather vague -- what was improved? |
@willmcgugan changed the title, expanded the description, and clarified the outstanding issue 4039. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a couple more comments - sorry! EDIT: Ignore 😆
@darrenburns ignored! But did you review at all? It doesn't show neither an accepted nor a change requested. |
Nope - I requested changes on the wrong PR. I didn't review this one. |
@rodrigogiraoserrao Your tests are essentially snapshot tests. They are as likely to catch any change to the tokenizer, as they are to catch errors. Suggest we have a test which takes CSS then checks that the expected styles are set on the widget. If that breaks in the future we know that it is a genuine issue, and not just that we have made an inconsequential changed to the tokenizer. |
Isn't that true of all tokenizer tests?
Then, doesn't that mean the test can fail for many different reasons that are completely orthogonal to the thing I'm trying to test, which is that the tokenizer knows what to do with these tokens? |
Yes, which is why we don't need more.
That's not what we should be testing. We should be testing the CSS that didn't work before, and now does. The output from the tokenizer does not prove that the CSS is processes as expected. |
Addresses review feedback. See the three comments starting at #4040 (comment).
@willmcgugan got it. See 3c18e47. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Fixes #3969 and fixes #3999.
Changes the tokenizer to accept lists of selectors in a nested context and also to accept styles after nested scopes.
While working on the above, we opened #4039 because the tokenizer isn't yet clever enough to distinguish a selector with a pseudo-class from a rule with a value in a nested context, if the selector + pseudo-class doesn't use the
&
operator. (See my comment and the interactions under it.)