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

Reformat SASS files with prettier #29566

Closed
wants to merge 2 commits into from
Closed

Conversation

blowery
Copy link
Contributor

@blowery blowery commented Dec 18, 2018

The results of running prettier over all of the scss files in the repo.

Changes proposed in this Pull Request

  • Reformat scss files with prettier

Testing instructions

  • Everything should look the way it did before
  • CSS output shouldn't really change

@matticbot
Copy link
Contributor

@blowery blowery self-assigned this Dec 18, 2018
@blowery blowery added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] Needs e2e Testing Framework [Type] Janitorial labels Dec 18, 2018
@@ -7,15 +7,14 @@
z-index: z-index( 'root', '.popover' );
position: absolute;
top: 0;
left: 0 #{"/*rtl:ignore*/"};
right: auto #{"/*rtl:ignore*/"};
left: 0 #{'/*rtl:ignore*/'};
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'm pretty sure this is using interpolation to insert an actual comment for later processing by rtlcss. I don't think moving from " to ' has any effect.

The results of running prettier over all of the scss files in the repo.
@blowery blowery force-pushed the update/sass-formatting branch from 40a0005 to ae70049 Compare December 19, 2018 13:57
@blowery
Copy link
Contributor Author

blowery commented Dec 19, 2018

Added #29604 as an optional add-on to this PR, which updates our stylelint rules a touch and runs --fix on everything.

@blowery
Copy link
Contributor Author

blowery commented Dec 19, 2018

Hrm, turns out prettier's SASS support has a few nasty bugs:
https://github.com/prettier/prettier/issues?utf8=%E2%9C%93&q=is%3Aissue+is%3Aopen+scss

Especially:
prettier/prettier#5603 - busted single line comments
prettier/prettier#4878 - same
prettier/prettier#4659 - same

The single line comment bug broke our z-index functions when i tried to apply prettier to it. :/
The first rule of a formatter has to be "do no harm" and prettier is failing that.

Maybe we don't want to use prettier for SASS just yet...?

cc @sirreal @Automattic/team-calypso

@blowery
Copy link
Contributor Author

blowery commented Dec 19, 2018

To repro the bugs in our codebase, run prettier against assets/stylesheets/shared/functions/_z-index.scss. Then try to run npm run build-css and you'll see it fail. The problem is the single line comments in the map getting pulled together and a map index ends up at the end of comment, breaking the syntax of the map.

@blowery
Copy link
Contributor Author

blowery commented Dec 21, 2018

Closing in favor of #29697

@blowery blowery closed this Dec 21, 2018
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 21, 2018
@belcherj
Copy link
Contributor

Prettier tanks on: assets/stylesheets/shared/functions/_z-index.scss

@jsnajdr jsnajdr deleted the update/sass-formatting branch January 7, 2019 12:30
@jsnajdr
Copy link
Member

jsnajdr commented Jan 7, 2019

I spent some time hacking Prettier over the holidays, and came up with two fixes for SCSS formatting:

Some other bugs still remain, including some that break code rather than just formatting it incorrectly. The most severe is prettier/prettier#5603. There I posted an analysis of the causes, but a fix won't be easy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants