-
Notifications
You must be signed in to change notification settings - Fork 148
[FEATURE] Parse container queries #1400
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
Conversation
JakeQZ
left a comment
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.
Could you also add a test to confirm the feature - ideally introducing an AtRuleTest TestCase? And add an entry to CHANGELOG.md?
Thanks.
013f3ef to
61977f9
Compare
|
Thanks! Added a test here, it's a little difficult to test being an interface so it just checks the value of the property, but open to any better ideas you have. |
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.
Thanks again.
it's a little difficult to test being an interface so it just checks the value of the property
My apologies. When looking at the diffs without full context, I was thinking that AtRule was a class.
The test you have added I think a is good to check to have for the internal behaviour (and fulfills the criteria: fails without the change; passes with). Though it seems PHPStan does not like a @covers annotation for an interface.
but open to any better ideas you have.
I've now done a bit of digging around the code base and existing tests and found that we have tests/CSSList/AtRuleBlockListTest.php. This has a test method parsesSyntacticallyCorrectAtRuleInStrictMode which checks that the at-rule is not discarded.
So I think to test the external beviour, it would good to have the provideSyntacticallyCorrectAtRule() data provider also provide a valid @contianer rule, which I expect would fail without the change. I think that would be sufficient (it's all we have for the other at-rules).
@oliverklee, do you have any other comments?
| */ | ||
| public function blockRulesConstantIsCorrect(): void | ||
| { | ||
| self::assertEqualsCanonicalizing( |
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.
Good choice of assert method - it allows the components to be in any order.
61977f9 to
45830cd
Compare
|
Aha! Thanks, good find -- I've added a |
JakeQZ
left a comment
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.
LGTM now. Thanks <3
Adds support for CSS container queries.
https://developer.mozilla.org/en-US/docs/Web/CSS/Guides/Containment/Container_queries