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

fix(@clayui/css): Use prettier to format Clay CSS #3069

Merged
merged 5 commits into from
Apr 7, 2020

Conversation

pat270
Copy link
Member

@pat270 pat270 commented Mar 25, 2020

fixes #2037

@bryceosterhaus I updated .prettierignore to exclude files related to the test site, Bootstrap source, and _global-functions.scss.

@pat270 pat270 force-pushed the clay-2037 branch 4 times, most recently from b894636 to 9a7250f Compare March 25, 2020 22:01
@coveralls
Copy link

coveralls commented Mar 25, 2020

Pull Request Test Coverage Report for Build 4698

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 78.536%

Totals Coverage Status
Change from base Build 4695: 0.0%
Covered Lines: 2297
Relevant Lines: 2726

💛 - Coveralls

.prettierignore Outdated
.codesandbox
.git
.github
.storybook
Copy link
Member

Choose a reason for hiding this comment

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

How come all of these got added? I think we should still include all of these except for the .git folder.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't realize these were configurations we added. I assumed these were autogenerated similar to .cache on clayui.com

.prettierignore Outdated
packages/clay-css/src/scss/bootstrap/**/**/**/*.scss
packages/clay-css/**/**/**/**/**/*.js
packages/clay-css/**/**/**/**/**/*.md
packages/clay-css/**/**/**/**/**/*.json
Copy link
Member

Choose a reason for hiding this comment

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

Also are we able to format the js md and json in clay? Or are those ignored for a specific reason?

Copy link
Member Author

Choose a reason for hiding this comment

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

The test site uses several third-party js plugins. I didn't want to ignore them separately since the test site isn't a critical part of the Clay repo. For example, we shouldn't be formatting svg4everybody.js, clay-css/src/js/site/jquery.ba-outside-events.min.js, sassdoc-theme-clay-css/dist/index.js, sassdoc-theme-clay-css/dist/nunjucks.js, etc.

If it's something you want I can add it. I was thinking along the lines of why process files that don't really matter?

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need all those repeated ** — one is equivalent to any number of intervening directories (full syntax described here).

For example:

  • packages/clay-css/**/*.js is the same as packages/clay-css/**/**/**/**/**/*.js

If you really want to say "match exactly 5 levels down" you would use a single * at each level:

  • packages/clay-css/*/*/*/*/*/*.js

If you want to ignore dist-like third-party stuff like the stuff you list you can either list it explicitly:

/packages/clay-css/dist
/packages/clay-css/src/js/site/jquery.*
# etc

or ignore everything then whitelist specific stuff:

/packages/clay-css/**/*.js
!packages/clay-css/specific/place/to/whitelist/**/*.js

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @wincent, I'll make these changes and push it up.

@pat270
Copy link
Member Author

pat270 commented Mar 26, 2020

@bryceosterhaus I removed .codesandbox, .github, and .storybook.
@wincent I couldn't get the whitelist pattern to work. Prettier isn't formatting the js files located in clay-css/tasks/lib. I tried !/packages/clay-css/tasks/lib, !packages/clay-css/tasks/lib, and several others.

@bryceosterhaus
Copy link
Member

@pat270 the js,json,md in clay-css should be fine, just wanted to verify. And the reason its not formatting tasks/lib is probably because we have lib declared in the prettierignore already.

@wincent
Copy link
Contributor

wincent commented Mar 27, 2020

And the reason its not formatting tasks/lib is probably because we have lib declared in the prettierignore already.

That's right. Given patterns like these:

lib
packages/clay-css/src/js/popper.js

Any lib directory at any level will get ignored (which means all of its descendents get ignored too). Generally, it's good hygiene to be as specific as possible, so I would anchor a lot of these patterns to the root with a leading /; ie that second line above would be:

/packages/clay-css/src/js/popper.js

And if we want to ignore lib everywhere but unignore it in a specific place, that is totally doable:

lib
!/some/other/lib

@pat270
Copy link
Member Author

pat270 commented Mar 30, 2020

@bryceosterhaus @wincent here is the updated .prettierignore

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 30, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 7d65ec6:

Sandbox Source
practical-frost-nned6 Configuration
stupefied-sound-gtv41 Configuration

Copy link
Member

@bryceosterhaus bryceosterhaus left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@pat270
Copy link
Member Author

pat270 commented Apr 7, 2020

Fixed conflicts.

@bryceosterhaus
Copy link
Member

@pat270 I think we can go ahead and merge this whenever you feel comfortable with it since this is likely gonna keep conflicting with every new pr merged. If @wincent has anything to add we can add it in a new pr.

@pat270 pat270 merged commit 0273c22 into liferay:master Apr 7, 2020
@pat270
Copy link
Member Author

pat270 commented Apr 7, 2020

@bryceosterhaus sounds good. Merging.

@pat270 pat270 deleted the clay-2037 branch April 22, 2020 23:14
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.

Use Prettier to format SCSS
4 participants