-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Fixed scss function arguments on different lines in maps #9128 #9184
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
Fixed scss function arguments on different lines in maps #9128 #9184
Conversation
alexander-akait
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.
Please add tests
|
Please add tests |
| ( | ||
| "print-only": "only print", | ||
| "screen-only": "only screen", | ||
| "xs-only": "only screen and (max-width: #{map-get($grid-breakpoints, "sm")-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.
I think it is some ugly, more readable (with spaces):
#{map-get($grid-breakpoints, "sm") - 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.
Sure will make the changes
| "sm-only": "only screen and (min-width: #{map-get($grid-breakpoints, "sm")}) and (max-width: #{map-get($grid-breakpoints, "md") - 1})", | ||
| ), | ||
| $display-breakpoints | ||
| ); No newline at end of file |
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.
Can we add couple test more? the harder is the 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.
sure, I will try to add. Do you have anything in mind?
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.
Something difficult with sass maps/list with interpolation
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.
added new testcases
| "bold": 700, | ||
| ); | ||
| @each $name, $boldness in $icons { | ||
| .text- #{ $name } { |
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.
Bug?
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 guess no. checked in the playground. Over there also it is formatting in the same way link
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.
Let's add/fix two cases here in:
.text-#{ $name }
and
.text #{ $name }
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.
sorry, didn't get your point. Can you please explain a bit.
.text-
#{ $name }
is formatting into
.text- #{ $name }
do you think it a bug? because we can't just assume that if it ends with "-", we have to merge the next text without space. The class name can also be just "test-", right?
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.
No, it is not bug, just put in tests:
@each
$size
in
$sizes {
.icon
#{$size}
{
border
:
"#{$size + "px"}"
solid
red;
}
}
and
@each
$size
in
$sizes {
.icon-#{$size}
{
border
:
"#{$size + "px"}"
solid
red;
}
}
and
//map test
$font-weights
:
("regular": 400
,
"medium": 500,
"bold":
700);
@each
$name,
$boldness
in
$icons {
.text
#{
$name
} {
color: red;
font-weight:
"#{$boldness}"
}
}
//map test
$font-weights
:
("regular": 400
,
"medium": 500,
"bold":
700);
@each
$name,
$boldness
in
$icons {
.text-#{
$name
} {
color: red;
font-weight:
"#{$boldness}"
}
}
Instead
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, will make the changes
| commaGroup.groups = [ | ||
| stringifyNode({ | ||
| groups: commaGroup.groups, | ||
| }).trim(), |
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.
Not sure it is safe to trim here, why we do it?
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.
Stringify was adding extra '\n', so I am using trim
|
/cc @fisker @sosukesuzuki |
| "bold": 700, | ||
| ); | ||
| @each $name, $boldness in $icons { | ||
| .text #{ $name } { |
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.
.icon-#{$size} above didn't add space around
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.
@fisker and should not, you can write it in different ways (all valid):
.text foo { }
.text- foo {}
.text-foo {}
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 mean space inside #{}, this one has space around $name, but .icon-#{$size} don't.
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.
Yep, you are right
/cc @agamkrbit
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 well make the changes ..
so .icon-#{$size} should be like .icon-#{ $size } right ?
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 like we are keeping spaces on stable version.
Prettier 2.1.2
Playground link
--parser scssInput:
#{$a } {
color: red;
}
#{ $a} {
color: red;
}
#{ $a } {
color: red;
}
#{$a} {
color: red;
}Output:
#{$a } {
color: red;
}
#{ $a} {
color: red;
}
#{ $a } {
color: red;
}
#{$a} {
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.
yeah.
sosukesuzuki
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.
Thanks
|
I have a question #9224 (comment) |
|
Do you think we should fix this by a solution fix both this and #9224? Update: Found a old issue #2734, it seems already fixed by the parser shellscape/postcss-values-parser#37, unfortunately we are still using a old version. |
|
@fisker I think we can merge, I am afraid we can't update parser, because it is broken in many cases, also [email protected]/8.2 want to include built-in parser for selectors and values, maybe better to wait it Yes, for both |
|
Looks like we forgot about this one. |
Fixed bug #9128
docs/directory)changelog_unreleased/*/pr-XXXX.mdfile followingchangelog_unreleased/TEMPLATE.md.✨Try the playground for this PR✨