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

Issue(1147): Add option to pretty print html components #1229

Merged
merged 5 commits into from May 18, 2019

Conversation

addisonssense
Copy link
Contributor

This PR is to implement pretty printing for html components.

Fixes #1147

Copy link

@quinnlangille quinnlangille left a comment

Choose a reason for hiding this comment

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

Yes! I've been waiting for this 🥇

@eddyerburgh
Copy link
Member

Thank you for the PR, as per my comment in the issue: can you make pretty-printing the default behavior?

@addisonssense
Copy link
Contributor Author

@eddyerburgh done! I have updated the existing test to use the prettyPrint: false option. If its better to update the html being asserted instead of using the flag, I can definitely do that instead. I figured this method was less intrusive

Copy link

@bilyes bilyes left a comment

Choose a reason for hiding this comment

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

Very cool feature at a relatively low cost. Thanks for the initiative @addisonssense !

@briwa
Copy link
Contributor

briwa commented May 15, 2019

@addisonssense I think existing tests should be updated with prettyPrint: true instead of false, since it should reflect the default behavior of .html? It will be consistent for future tests that use .html, since they would probably be written with the default behavior in mind (pretty-printed).

Just my $0.02 ✌️ But anyhow, nice PR!

@addisonssense
Copy link
Contributor Author

@briwa I think you make a good point. I'll do that 👍

@TheJaredWilcurt
Copy link

A few months ago I proposed a different approach to this. jest-serializer-vue has options built in, but the options do not format (or "pretty print") the code very well.

TLDR:

  • Change the default settings being passed in to pretty to these:
    const options = {
      indent_char: ' ',
      indent_inner_html: true,
      indent_size: 2,
      inline: [],
      sep: '\n',
      unformatted: ['code', 'pre']
    };
  • Allow users to pass in their own options object. (Not sure where they would do this, package.json?)
  • Have instructions of how to pass in the object with the previous settings (so users can opt out of breaking changes).

This allows us to change the defaults again in the future without worrying about breaking people's tests, as the defaults can be documented for each version.

@eddyerburgh
Copy link
Member

I intend to implement the changes in jest-serializer-vue but I haven't had time to work on it.

jest-serializer-vue will be formatted more heavily, whereas vue-test-utils can just use the defaults.

@addisonssense thanks for updating, but I wasn't clear in my previous comment. I don't think there should be an option to pretty-print or not. I don't think it should be configurable, and pretty printing should always be the default behavior.

Can you update the PR to remove the prettyPrint option entirely?

@addisonssense
Copy link
Contributor Author

@eddyerburgh done! Pretty printing is now the default. The option to disable it is removed.

Copy link
Member

@eddyerburgh eddyerburgh left a comment

Choose a reason for hiding this comment

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

Thanks :)

@eddyerburgh eddyerburgh merged commit 99336c4 into vuejs:dev May 18, 2019
@vue-bot
Copy link

vue-bot commented May 18, 2019

Hey @addisonssense, thank you for your time and effort spent on this PR, contributions like yours help make Vue better for everyone. Cheers! 💚

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.

Pretty-print html() output by default
7 participants