Skip to content

Conversation

kevincox
Copy link
Contributor

@kevincox kevincox commented Feb 21, 2020

This reduces the size of the HTML and improves readability.

The current default behaviour is kept the same but an option is provided
to enable the new behaviour. On a future version bump I would flip the
default value.

Notable Changes

✏️ Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request. If it fixes a bug or resolves a feature request, be sure to link to that issue down below.

Commit Message Summary (CHANGELOG)

commit message body...

Type

ℹ️ What types of changes does your code introduce?

👉 Put an x in the boxes that apply and delete all others

  • Feature

SemVer

ℹ️ What changes to the current semver range does your PR introduce?

👉 Put an x in the boxes that apply and delete all others

  • Breaking Change (:label: Major)

It depends what guarantees about the output are provided. This is probably minor as documented but is very likely to break some downstream tests so should probably be treated as a major change.

Checklist

ℹ️ You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. This is a reminder of what we are going to look for before merging your code.

👉 Put an x in the boxes that apply and delete all others.

  • Lint and unit tests pass with my changes
  • I have added tests that prove my fix is effective/works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes are merged and published in downstream modules

@coveralls
Copy link

coveralls commented Feb 21, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling 3492483 on kevincox:unquote into 15d30db on posthtml:master.

Copy link
Member

@anikethsaha anikethsaha left a comment

Choose a reason for hiding this comment

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

Thanks for submitting PR.

This feature looks good to me, just need to see the mentioned tests.

This reduces the size of the HTML and improves readability.

The current default behaviour is kept the same but an option is provided
to enable the new behaviour. On a future version bump I would flip the
default value.
Copy link
Member

@anikethsaha anikethsaha left a comment

Choose a reason for hiding this comment

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

🚀

@anikethsaha
Copy link
Member

Although, this is a minification feature. I think this will be the best fit for htmlnano as renderer should not do any minification.

@kevincox What do you think?

cc @Scrum

@kevincox
Copy link
Contributor Author

kevincox commented Feb 21, 2020

I was implementing this so that it could be used by htmlnano. It seems to me that currently posthtml-render has no way to allow emit this syntax which htmlnano would want.

While in my case I am doing this for minification I would also argue that this syntax is more readable and some people would prefer to have it even when not doing minification.

@anikethsaha
Copy link
Member

It seems to me that currently posthtml-render has no way to allow emit this syntax which htmlnano would want.

Yeah, omitting these kinds of behavior can not be done by the plugin as of now.

Will merge this after one more approval 👍

@kevincox
Copy link
Contributor Author

Thinking about it a bit more htmlnano doesn't currently do any modification to the formatting of the HTML, only modification of the AST. Since this change doesn't affect the AST at all it makes even more sense in the renderer than it does in htmlnano.

@anikethsaha
Copy link
Member

After merging this, I think it should be added in htmlnano's docs as well about this options 👍

Copy link
Member

@voischev voischev left a comment

Choose a reason for hiding this comment

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

thx

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.

4 participants