Skip to content
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

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
);
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
Sponsor 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
Sponsor 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
Sponsor 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
Sponsor Member

fisker commented Sep 17, 2020

I have a question #9224 (comment)

@fisker
Copy link
Sponsor 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 postcss@8.1/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
Sponsor Member

fisker commented Nov 24, 2022

Looks like we forgot about this one.

@fisker fisker self-assigned this Nov 24, 2022
@fisker fisker merged commit 4a1e32a into prettier:main Nov 24, 2022
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Dec 23, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [prettier](https://prettier.io) ([source](https://github.com/prettier/prettier)) | devDependencies | patch | [`2.8.0` -> `2.8.1`](https://renovatebot.com/diffs/npm/prettier/2.8.0/2.8.1) |

---

### Release Notes

<details>
<summary>prettier/prettier</summary>

### [`v2.8.1`](https://github.com/prettier/prettier/blob/HEAD/CHANGELOG.md#&#8203;281)

[Compare Source](prettier/prettier@2.8.0...2.8.1)

[diff](prettier/prettier@2.8.0...2.8.1)

##### Fix SCSS map in arguments ([#&#8203;9184](prettier/prettier#9184) by [@&#8203;agamkrbit](https://github.com/agamkrbit))

<!-- prettier-ignore -->

```scss
// Input
$display-breakpoints: map-deep-merge(
  (
    "print-only": "only print",
    "screen-only": "only screen",
    "xs-only": "only screen and (max-width: #{map-get($grid-breakpoints, "sm")-1})",
  ),
  $display-breakpoints
);

// Prettier 2.8.0
$display-breakpoints: map-deep-merge(
  (
    "print-only": "only print",
    "screen-only": "only screen",
    "xs-only": "only screen and (max-width: #{map-get($grid-breakpoints, " sm
      ")-1})",
  ),
  $display-breakpoints
);

// Prettier 2.8.1
$display-breakpoints: map-deep-merge(
  (
    "print-only": "only print",
    "screen-only": "only screen",
    "xs-only": "only screen and (max-width: #{map-get($grid-breakpoints, "sm")-1})",
  ),
  $display-breakpoints
);
```

##### Support auto accessors syntax ([#&#8203;13919](prettier/prettier#13919) by [@&#8203;sosukesuzuki](https://github.com/sosukesuzuki))

Support for [Auto Accessors Syntax](https://devblogs.microsoft.com/typescript/announcing-typescript-4-9/#auto-accessors-in-classes) landed in TypeScript 4.9.

(Doesn't work well with `babel-ts` parser)

<!-- prettier-ignore -->

```tsx
class Foo {
  accessor foo: number = 3;
}
```

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC41MS4wIiwidXBkYXRlZEluVmVyIjoiMzQuNzAuNCJ9-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1671
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
medikoo pushed a commit to medikoo/prettier-elastic that referenced this pull request Jan 4, 2024
prettier#9184)

Co-authored-by: agamkrbit <agamkrbit123@gmail.com>
Co-authored-by: fisker Cheung <lionkay@gmail.com>
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.

None yet

5 participants