-
-
Notifications
You must be signed in to change notification settings - Fork 550
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
fix(css): align CSS value list format with Prettier #5334
base: main
Are you sure you want to change the base?
Conversation
CodSpeed Performance ReportMerging #5334 will not alter performanceComparing Summary
|
498013c
to
f30af76
Compare
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.
Great fix! A couple of things left:
- add more comments to document the new layout variant
- add a changeset
at_group_boundary = | ||
is_comma && matches!(layout, ValueListLayout::OneGroupPerLine); | ||
|
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.
You might want to add a comment that explains why we change the value of at_group_boundary
after the first iteration of the loop.
@@ -160,6 +168,16 @@ pub(crate) enum ValueListLayout { | |||
/// sans-serif; | |||
/// ``` | |||
OnePerLine, | |||
|
|||
/// Separate values by comma into multiple groups, and print each group on a single line |
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.
In the PR description, you explained when this layout was applied. That is essential knowledge and we want to keep it around, so please add that information here :)
} | ||
group_size += 1; | ||
} | ||
group_count += 1; |
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.
Why do we add another item after the loop? This means that if the list
is empty, group_count
is always one here, which doesn't add up.
group_size += 1; | ||
} | ||
group_count += 1; | ||
max_group_size = cmp::max(group_size, max_group_size); |
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.
Why do we calculate max_group_size
again? You might want to consider adding a comment explaining why there's this logic because it's unclear.
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.
AWESOME!!! 🙌
Summary
Related issue: #5307
Currently biome's formatting of CSS values is different from Prettier
example link
Align biome with Prettier
Related code:
crates/biome_css_formatter/src/utils/component_value_list.rs
Align with Prettier logic. (For detail see below section)
Added a layout called
OneGroupPerLine
When a value list satisfy below conditions, will format using
OneGroupPerLine
Render
OneGroupPerLine
at_group_boundary
tofalse
, and iterate through the value list:at_group_boundary
istrue
at_group_boundary
totrue
, else, set tofalse
Prettier logic
Here is a brief description of how Prettier handle it:
### Process ASTRelated code:
https://github.com/prettier/prettier/blob/3.5.3/src/language-css/parse/parse-value.js
Prettier first use PostcssValuesParser to parse CSS values, the AST is similar to what we have in biome
Then, Prettier calls
parseNestedValue
to process the AST as follows:paren_group
andcomma_group
paren_group
, any new parenthesis (like inurl()
) will create a new layer ofparen_group
paren_group
, by default everything is wrapped in acomma_group
. When find a comma, push in the currentcomma_group
, rest items are added to a newcomma_group
.paren_group
andcomma_group
: if the group contains only 1 item, remove the group wrapper and expose the item directly.Print
Related code:
https://github.com/prettier/prettier/blob/3.5.3/src/language-css/print/parenthesized-value-group.js#L62
Related to reported bug, one of the cases that Prettier will print each value to a single line is:
Condition 2 implies that there are >= 2 values. (Otherwise the paren_group wrapper will be removed).
Condition 3 implies that at least one of the values has >= 2 parts. (Otherwise the comma_group wrapper will be removed).
Test Plan
Snapshot updated
Add new test
crates/biome_css_formatter/tests/specs/css/value_one_group_per_line.css
font-family
part still useFill
. Because each group separated by comma only contain 1 value, not triggerOneGroupPerLine
. And the value count <= 12, not triggerOnePerLine
.-apple-system
as 2 values:-
+apple-system
, thus trigger itsOneGroupPerLine
logic.