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

Preserve single new-line gaps between elements #58

Merged
merged 3 commits into from
Oct 28, 2018

Conversation

rndmerle
Copy link
Contributor

Hi :)

I tried to get a stab at #43

I'm really not sure it's done the best it could be, but it sounds decent.
If you think of a better way, or at least better names, please tell me.

I didn't know if you wanted this feature behind an option. It's still possible.

@StarpTech
Copy link
Member

Hi @rndmerle thank you for PR. I will check it.

packages/hast-util-from-webparser/index.ts Outdated Show resolved Hide resolved
packages/hast-util-from-webparser/index.ts Outdated Show resolved Hide resolved
packages/hast-util-from-webparser/test/index.test.ts Outdated Show resolved Hide resolved
packages/hast-util-from-webparser/index.ts Outdated Show resolved Hide resolved
@@ -116,7 +116,8 @@ function collapsable(node) {
/* Collapse to spaces, or newlines if they’re in a run. */
function collapseToNewLines(value) {
var result = String(value).replace(/\s+/g, function($0) {
return $0.indexOf('\n') === -1 ? ' ' : '\n'
const newLinesCount = ($0.match(/\n/g) || []).length
Copy link
Member

Choose a reason for hiding this comment

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

Could you add some comments why 2 newlines are added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a leftover from when I was exploring some solutions.
I'll check but it seems unneeded.

packages/rehype-minify-whitespace/test.js Outdated Show resolved Hide resolved
@rndmerle
Copy link
Contributor Author

Thanks for the review, I'll check it out in detail a bit later ;)

@rndmerle
Copy link
Contributor Author

Code has been improved ;)
Nice catch, minify-whitespace changes were unneeded after all.

@StarpTech
Copy link
Member

Good job! 👍

@StarpTech StarpTech merged commit 4bee01a into Prettyhtml:master Oct 28, 2018
@rndmerle rndmerle deleted the preserved-gaps branch October 28, 2018 16:29
@StarpTech
Copy link
Member

@rndmerle new version was released and can be checked on the playground in ~5min.

@rndmerle
Copy link
Contributor Author

I tested it on some big forms. It works wonder 😎

It's a bit off-topic but I just noticed that the playground is not indenting the content of <script> and <style> elements while the prettyhtml client (0.5.0) does.
Opening up and issue or is it somehow normal?

@StarpTech
Copy link
Member

StarpTech commented Oct 28, 2018

@rndmerle this is a known limitation because prettyhtml wasn't built for the web (even not with browserify) the option prettier is set to false in the playground. If you have an idea feel free to fix it 😄 I hadn't the time to investigate.

@StarpTech
Copy link
Member

@rndmerle would you like to join to the prettyhtml team?

@rndmerle
Copy link
Contributor Author

Thanks much for asking but I've few spare time to contribute here and there and I prefer not to feel entitled to a particular project, nor promise I can spend more time when I maybe won't.
Even if Prettyhtml is an interesting project.
That being said I'll be happy to contribute again if I get the chance.

@juanesarango
Copy link

I was just looking for how to fix this issue as well. Thanks for fixing this @rndmerle

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.

3 participants