Skip to content

Conversation

@agamkrbit
Copy link
Contributor

@agamkrbit agamkrbit commented Sep 10, 2020

Fixed bug #9128

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory)
  • (If the change is user-facing) I’ve added my changes to changelog_unreleased/*/pr-XXXX.md file following changelog_unreleased/TEMPLATE.md.
  • I’ve read the contributing guidelines.

Try the playground for this PR

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add tests

@alexander-akait
Copy link
Member

Please add tests

(
"print-only": "only print",
"screen-only": "only screen",
"xs-only": "only screen and (max-width: #{map-get($grid-breakpoints, "sm")-1})",
Copy link
Member

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}

Copy link
Contributor Author

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
Copy link
Member

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

Copy link
Contributor Author

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?

Copy link
Member

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

Copy link
Contributor Author

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 } {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug?

Copy link
Contributor Author

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

Copy link
Member

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 }

Copy link
Contributor Author

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?

Copy link
Member

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

Copy link
Contributor Author

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(),
Copy link
Member

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?

Copy link
Contributor Author

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

@alexander-akait
Copy link
Member

/cc @fisker @sosukesuzuki

"bold": 700,
);
@each $name, $boldness in $icons {
.text #{ $name } {
Copy link
Member

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

Copy link
Member

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 {}

Copy link
Member

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.

Copy link
Member

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

Copy link
Contributor Author

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 ?

Copy link
Member

@fisker fisker Sep 16, 2020

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 scss

Input:

#{$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;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah.

Copy link
Member

@sosukesuzuki sosukesuzuki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@fisker
Copy link
Member

fisker commented Sep 17, 2020

I have a question #9224 (comment)

@fisker
Copy link
Member

fisker commented Sep 23, 2020

@evilebottnawi

Do you think we should fix this by a solution fix both this and #9224?
It's more like "interpolation inside quoted string".


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.

@alexander-akait
Copy link
Member

alexander-akait commented Sep 23, 2020

@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

Base automatically changed from master to main January 23, 2021 17:13
@fisker
Copy link
Member

fisker commented Nov 24, 2022

Looks like we forgot about this one.

@fisker fisker self-assigned this Nov 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants