Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Run prettier on HTML files #5839

Merged
merged 1 commit into from
Nov 23, 2018
Merged

Run prettier on HTML files #5839

merged 1 commit into from
Nov 23, 2018

Conversation

sibiraj-s
Copy link
Contributor

@sibiraj-s sibiraj-s commented Nov 17, 2018

Prettier now supports HTML, Added html to lint-statged.

updated prettier to v1.15.2

@Timer Timer added this to the 2.1.2 milestone Nov 23, 2018
@Timer
Copy link
Contributor

Timer commented Nov 23, 2018

Fantastic, thanks!

@Timer Timer merged commit 85a8a22 into facebook:master Nov 23, 2018
<noscript>
You need to enable JavaScript to run this app.
</noscript>
<noscript> You need to enable JavaScript to run this app. </noscript>
Copy link
Contributor

Choose a reason for hiding this comment

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

I could have sworn I commented on this earlier. This all looks fine but the spacing inside this tag is weird. It's not a big deal, just kind of weird choice on behalf of prettier.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh wow, that is weird. I wonder if it's a bug.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's technically correct because the whitespace that was there when things were on multiple lines does get turned into a single space as that's just how whitespace in HTML works. But preserving it in this case seems like an odd choice.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, we can probably safely remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely. I think Prettier is preserving it because in some cases you might be relying on that space. Think about how whitespace works in JSX in a multiline block. So I think Prettier is just playing it safe and leaving it there for you. But we definitely don't need it in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've removed the extra spaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is an open issue in prettier for handling whitespace efficiently prettier/prettier#5439

dardub added a commit to OffBase/create-react-app that referenced this pull request Nov 27, 2018
* upstream/master: (210 commits)
  Support setupTests.ts (facebook#5698)
  Remove unnecessary whitespace in template HTML
  Run prettier on HTML files (facebook#5839)
  Some Grammar fixes (facebook#5858)
  Fix link to page about running tests (facebook#5883)
  fix: make typescriptformatter support 0.5 of fork checker (facebook#5879)
  Always test with the latest stable Node version on Travis (facebook#5546)
  Fix propertyDecorator test
  Upgrade babel deps
  Fix annotated var test
  Fix TypeScript decorator support (facebook#5783)
  fix: add `sideEffects: false` to react-error-overlay (facebook#5451)
  Add allowESModules option to babel-preset-react-app (facebook#5487)
  Make named-asset-import plugin work with export-as syntax (facebook#5573)
  React native repository updated in README.md (facebook#5849)
  extra polyfills must be included manually (facebook#5814)
  Rename 'getting started' link to 'docs' (facebook#5806)
  docs: Simplify installing Storybook with npx (facebook#5788)
  Don't polyfill fetch for Node -- additional files (facebook#5789)
  docs: Change Storybook install documentation (facebook#5779)
  ...
mrmckeb pushed a commit to BeameryHQ/bmr-react-scripts that referenced this pull request Dec 3, 2018
timsnadden added a commit to timsnadden/create-react-app that referenced this pull request Dec 7, 2018
* master: (39 commits)
  Added extension to .eslintrc (facebook#5988)
  Update links to docs in all package README files (facebook#5912)
  Use https for linked images in docs to fix mixed content warnings (facebook#5985)
  Improve error messaging in verifyPackageTree.js (facebook#5974)
  Add removeItem to localStorage mock in docs (facebook#5919)
  Add SASS_PATH instructions to Sass docs (facebook#5917)
  Suggest a different default for speed reasons (facebook#5959)
  Add pre-eject message about new features in v2 (facebook#5954)
  Add netlify.toml to prepare for deploy to netlify facebook#5807 (facebook#5930)
  Correct some comments (facebook#5927)
  Add note to docs about using Sass and Flow together (facebook#5823)
  Update PWA link in README (facebook#5907)
  Add placeholders to template README for bit.ly links. (facebook#5808)
  Disable copy to clipboard in cra --info (facebook#5905)
  Support setupTests.ts (facebook#5698)
  Remove unnecessary whitespace in template HTML
  Run prettier on HTML files (facebook#5839)
  Some Grammar fixes (facebook#5858)
  Fix link to page about running tests (facebook#5883)
  fix: make typescriptformatter support 0.5 of fork checker (facebook#5879)
  ...
@lock lock bot locked and limited conversation to collaborators Jan 18, 2019
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants