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

Format SCSS maps like CSS rules #3070

Merged
merged 2 commits into from Oct 22, 2017
Merged

Format SCSS maps like CSS rules #3070

merged 2 commits into from Oct 22, 2017

Conversation

asmockler
Copy link
Contributor

Resolves #3056

This makes prettier format SCSS maps like CSS rules (line breaks after each rule). e.g.

$map: (color: #111111, text-shadow: 1px 1px 0 salmon)

will format to

$map: (
  color: #111111,
  text-shadow: 1px 1px 0 salmon
);

join(concat([",", line]), path.map(print, "groups"))
join(
concat([",", parent.type === "value-value" ? hardline : line]),
path.map(print, "groups")
Copy link
Member

Choose a reason for hiding this comment

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

As you've probably noticed, the AST nodes in CSS/SCSS/Less are super vague and super general. While this PR looks good, I'm worried about what unintended side effects this might/will have. Our tests aren't very comprehensive. But I guess we could also just release it and do the regular couple of point releases afterwards with CSS regression fixes ¯\_(ツ)_/¯

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it help if we checked startsWith('$') on the parent value? If that doesn't give any more assurance, I am fine fielding any regressions that arise related to this.

Copy link
Member

@azz azz left a comment

Choose a reason for hiding this comment

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

I think the startsWith check is a good idea, but otherwise LGTM.

@asmockler
Copy link
Contributor Author

Added the startsWith check; it now reaches up 2 nodes and checks for a css-decl+ startsWith("$") instead of just value-value 👍

const declaration = path.getParentNode(2);
const isMap =
declaration.type === "css-decl" && declaration.prop.startsWith("$");

Copy link
Member

Choose a reason for hiding this comment

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

Might want to be a bit defensive and prefix with declaration && . It might not exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call - just updated.

@azz azz merged commit c6d97ab into prettier:master Oct 22, 2017
@azz
Copy link
Member

azz commented Oct 22, 2017

Thanks!

@asmockler asmockler deleted the scss-maps branch October 22, 2017 05:12
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jan 19, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants