-
-
Notifications
You must be signed in to change notification settings - Fork 878
fix(css/formatter): comment placement in lists #9261
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| --- | ||
| "@biomejs/biome": patch | ||
| --- | ||
|
|
||
| Fixed [#8409](https://github.com/biomejs/biome/issues/8409): CSS formatter now correctly places comments after the colon in property declarations. | ||
|
|
||
| Previously, comments that appeared after the colon in CSS property values were incorrectly moved before the property name: | ||
|
|
||
| ```css | ||
| /* Before (incorrect) */ | ||
| [lang]:lang(ja) { | ||
| /* system-ui,*/ font-family: | ||
| Hiragino Sans, | ||
| sans-serif; | ||
| } | ||
|
|
||
| /* After (correct) */ | ||
| [lang]:lang(ja) { | ||
| font-family: /* system-ui,*/ | ||
| Hiragino Sans, | ||
| sans-serif; | ||
| } | ||
| ``` | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -194,11 +194,13 @@ where | |||||||||||||||||||||||||||||
| let has_leading_newline = element.syntax().has_leading_newline(); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| if has_leading_newline { | ||||||||||||||||||||||||||||||
| dbg!("here1"); | ||||||||||||||||||||||||||||||
| write!(f, [hard_line_break()])?; | ||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||
| write!(f, [space()])?; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } else if at_group_boundary { | ||||||||||||||||||||||||||||||
| dbg!("here2"); | ||||||||||||||||||||||||||||||
| write!(f, [hard_line_break()])?; | ||||||||||||||||||||||||||||||
|
Comment on lines
+197
to
204
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove debug statements before merge. These 🧹 Proposed fix if has_leading_newline {
- dbg!("here1");
write!(f, [hard_line_break()])?;
} else {
write!(f, [space()])?;
}
} else if at_group_boundary {
- dbg!("here2");
write!(f, [hard_line_break()])?;📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||
| write!(f, [soft_line_break_or_space()])? | ||||||||||||||||||||||||||||||
|
|
@@ -227,8 +229,12 @@ where | |||||||||||||||||||||||||||||
| // This is also why `at_group_boundary` is initialized to `false` even when | ||||||||||||||||||||||||||||||
| // the layout is OneGroupPerLine: because the line break would be ignored | ||||||||||||||||||||||||||||||
| // if `at_group_boundary` were set to `true` initially. | ||||||||||||||||||||||||||||||
| at_group_boundary = | ||||||||||||||||||||||||||||||
| is_comma && matches!(layout, ValueListLayout::OneGroupPerLine); | ||||||||||||||||||||||||||||||
| at_group_boundary = is_comma | ||||||||||||||||||||||||||||||
| && matches!( | ||||||||||||||||||||||||||||||
| layout, | ||||||||||||||||||||||||||||||
| ValueListLayout::OneGroupPerLine | ||||||||||||||||||||||||||||||
| | ValueListLayout::OneGroupPerLineWithDanglingComments | ||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| Ok(()) | ||||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||||
|
|
@@ -272,6 +278,11 @@ where | |||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| write!(f, [group(&indent(&content))]) | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| ValueListLayout::OneGroupPerLineWithDanglingComments => { | ||||||||||||||||||||||||||||||
| // Dangling comments are formatted inline by the property's fmt_dangling_comments | ||||||||||||||||||||||||||||||
| // We only need to indent the values, no hard line break here | ||||||||||||||||||||||||||||||
| write!(f, [group(&indent(&values))]) | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
@@ -354,6 +365,15 @@ pub(crate) enum ValueListLayout { | |||||||||||||||||||||||||||||
| /// These conditions are inherited from Prettier, | ||||||||||||||||||||||||||||||
| /// see https://github.com/biomejs/biome/pull/5334 for a detailed explanation | ||||||||||||||||||||||||||||||
| OneGroupPerLine, | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| /// Similar to OneGroupPerLine, but formats dangling comments on the property inline | ||||||||||||||||||||||||||||||
| /// before the line break. Used when comments appear between the colon and values. | ||||||||||||||||||||||||||||||
| /// ```css | ||||||||||||||||||||||||||||||
| /// font-family: /* comment */ | ||||||||||||||||||||||||||||||
| /// Hiragino Sans, | ||||||||||||||||||||||||||||||
| /// sans-serif; | ||||||||||||||||||||||||||||||
| /// ``` | ||||||||||||||||||||||||||||||
| OneGroupPerLineWithDanglingComments, | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| fn should_preceded_by_softline<N, I>(node: &N) -> bool | ||||||||||||||||||||||||||||||
|
|
@@ -371,7 +391,7 @@ where | |||||||||||||||||||||||||||||
| /// printed compactly. | ||||||||||||||||||||||||||||||
| pub(crate) fn get_value_list_layout<N, I>( | ||||||||||||||||||||||||||||||
| list: &N, | ||||||||||||||||||||||||||||||
| _: &CssComments, | ||||||||||||||||||||||||||||||
| comments: &CssComments, | ||||||||||||||||||||||||||||||
| f: &CssFormatter, | ||||||||||||||||||||||||||||||
| ) -> ValueListLayout | ||||||||||||||||||||||||||||||
| where | ||||||||||||||||||||||||||||||
|
|
@@ -402,13 +422,27 @@ where | |||||||||||||||||||||||||||||
| .iter() | ||||||||||||||||||||||||||||||
| .any(|x| CssGenericDelimiter::cast_ref(x.syntax()).is_some()); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Check if the property name has trailing comments (comments between name and values) | ||||||||||||||||||||||||||||||
| // If so, we don't need to change the layout since the comments will be formatted | ||||||||||||||||||||||||||||||
| // inline with the property name, outside the value indent block | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Check if the parent property has trailing comments (comments between colon and values) | ||||||||||||||||||||||||||||||
| let parent_property = list.parent::<CssGenericProperty>(); | ||||||||||||||||||||||||||||||
| let has_trailing_comments = parent_property | ||||||||||||||||||||||||||||||
| .as_ref() | ||||||||||||||||||||||||||||||
| .is_some_and(|prop| !comments.trailing_comments(prop.syntax()).is_empty()); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // TODO: Check for comments, check for the types of elements in the list, etc. | ||||||||||||||||||||||||||||||
| if is_grid_property { | ||||||||||||||||||||||||||||||
| ValueListLayout::PreserveInline | ||||||||||||||||||||||||||||||
| } else if list.len() == 1 { | ||||||||||||||||||||||||||||||
| ValueListLayout::SingleValue | ||||||||||||||||||||||||||||||
| } else if use_one_group_per_line(css_property.as_deref(), list) { | ||||||||||||||||||||||||||||||
| ValueListLayout::OneGroupPerLine | ||||||||||||||||||||||||||||||
| if has_trailing_comments { | ||||||||||||||||||||||||||||||
| ValueListLayout::OneGroupPerLineWithDanglingComments | ||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||
| ValueListLayout::OneGroupPerLine | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } else if is_comma_separated | ||||||||||||||||||||||||||||||
| && value_count > 12 | ||||||||||||||||||||||||||||||
| && text_size >= TextSize::from(f.options().line_width().value() as u32) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| /* Comment after colon in property value */ | ||
| [lang]:lang(ja) { | ||
| font-family: /* system-ui,*/ Hiragino Sans, sans-serif; | ||
| } | ||
|
|
||
| /* Another case */ | ||
| .selector { | ||
| color: /* red, */ blue; | ||
| } | ||
|
|
||
| /* Another case */ | ||
| .selector { | ||
| color/* red, */: blue; | ||
| } | ||
|
|
||
| /* Comment before property name should still work */ | ||
| .selector { | ||
| /* comment */ | ||
| color: red; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| --- | ||
| source: crates/biome_formatter_test/src/snapshot_builder.rs | ||
| info: css/comments/property-value-comment.css | ||
| --- | ||
|
|
||
| # Input | ||
|
|
||
| ```css | ||
| /* Comment after colon in property value */ | ||
| [lang]:lang(ja) { | ||
| font-family: /* system-ui,*/ Hiragino Sans, sans-serif; | ||
| } | ||
| /* Another case */ | ||
| .selector { | ||
| color: /* red, */ blue; | ||
| } | ||
| /* Another case */ | ||
| .selector { | ||
| color/* red, */: blue; | ||
| } | ||
| /* Comment before property name should still work */ | ||
| .selector { | ||
| /* comment */ | ||
| color: red; | ||
| } | ||
| ``` | ||
|
|
||
|
|
||
| ============================= | ||
|
|
||
| # Outputs | ||
|
|
||
| ## Output 1 | ||
|
|
||
| ----- | ||
| Indent style: Tab | ||
| Indent width: 2 | ||
| Line ending: LF | ||
| Line width: 80 | ||
| Quote style: Double Quotes | ||
| Trailing newline: true | ||
| ----- | ||
|
|
||
| ```css | ||
| /* Comment after colon in property value */ | ||
| [lang]:lang(ja) { | ||
| font-family: /* system-ui,*/ | ||
| Hiragino Sans, | ||
| sans-serif; | ||
| } | ||
| /* Another case */ | ||
| .selector { | ||
| color: /* red, */ | ||
| blue; | ||
| } | ||
| /* Another case */ | ||
| .selector { | ||
| color /* red, */: blue; | ||
| } | ||
| /* Comment before property name should still work */ | ||
| .selector { | ||
| /* comment */ | ||
| color: red; | ||
| } | ||
| ``` |
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.
Inconsistent indentation in the "After" example.
Line 21 has extra leading whitespace (
sans-serifis indented more thanHiragino Sans). If this reflects actual formatter output, it may indicate the regression mentioned in the PR description. If it's a typo in the changeset, it should be corrected.📝 If this is a typo, proposed fix
/* After (correct) */ [lang]:lang(ja) { font-family: /* system-ui,*/ Hiragino Sans, - sans-serif; + sans-serif; }📝 Committable suggestion
🤖 Prompt for AI Agents