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

Text Editor: Adding new lines between blocks and inside block content #663

Merged
merged 2 commits into from
May 5, 2017

Conversation

youknowriad
Copy link
Contributor

Closes #633

This PR adds new lines between blocks and inside block delimeters if the content is not empty.

  • When parsing we trim the content of the blocks to avoid unecessary spaces, I think this is fine because the save method of each block return element or elements for all cases excluding the fallback block.

  • We also ignore fallback blocks with empty content.

@youknowriad youknowriad requested review from jasmussen and aduth May 4, 2017 15:09
@jasmussen
Copy link
Contributor

Yaay! I love this so much. MASSIVELY better.

And once we merge in #662, (for which I apologize), it'll be still better yet! :D

@jasmussen
Copy link
Contributor

jasmussen commented May 4, 2017

I wouldn't object to us going even further. I.e. do this:

<!-- wp:core/image -->
<figure>
<img src="https://cldup.com/vuGcj2VB8M.jpg" class="alignnone"/>
<figcaption>Beautiful landscape</figcaption>
</figure>
<!-- /wp:core/image -->

instead of

<!-- wp:core/image -->
<figure><img src="https://cldup.com/vuGcj2VB8M.jpg" class="alignnone"/><figcaption>Beautiful landscape</figcaption></figure>
<!-- /wp:core/image -->

But I'm also completely okay with us not going those extra steps. I imagine it gets very complex very quickly.

@youknowriad
Copy link
Contributor Author

I wouldn't object to us going even further

@jasmussen We may want to try an HTML beautifier for this. We need to check the performance and size of a library like this. Anyone aware of a good light HTML beautifier @aduth @iseulde ?

@ellatrix
Copy link
Member

ellatrix commented May 4, 2017

More parsing! 😱 😄

@jasmussen
Copy link
Contributor

We may want to try an HTML beautifier for this. We need to check the performance and size of a library like this. Anyone aware of a good light HTML beautifier

It sounds like we should hold off on that for now! As you were — let's not let perfect be the enemy of good! :D

@ellatrix
Copy link
Member

ellatrix commented May 4, 2017

With the parsing, I'm just kidding. But we could avoid it though if we only do it visual => text, and not beautify when loading from the database (leave it as they saved it).

@youknowriad youknowriad force-pushed the add/serialization-spacing branch from c7f4363 to c617670 Compare May 5, 2017 09:25
@youknowriad
Copy link
Contributor Author

I've added a beautifier @jasmussen. It's not the exact output you're proposing but really close. It has a lot of options we can tweak.

@jasmussen
Copy link
Contributor

OMG 😍

I love this so much. If it doesn't make things overly complex for the parsing, I really really dig this! ❤️❤️❤️❤️❤️

@youknowriad
Copy link
Contributor Author

Any technical 👀 here?

@ellatrix
Copy link
Member

ellatrix commented May 5, 2017

I'll have a quick look

@ellatrix
Copy link
Member

ellatrix commented May 5, 2017

Works great here, I have no concerns with it...

Just a question: would it not work to beautify the whole content instead of block per block?

@youknowriad
Copy link
Contributor Author

@iseulde It could but the difference is that we won't be able to control the new lines between the blocks, we'll have only a single new line instead of two between the different blocks.

@youknowriad youknowriad merged commit 1cb599d into master May 5, 2017
@youknowriad youknowriad deleted the add/serialization-spacing branch May 5, 2017 13:14
@aduth
Copy link
Member

aduth commented May 5, 2017

Should we have beautified the HTML in post-content.js?

@youknowriad
Copy link
Contributor Author

@aduth Technically it doesn't matter since it'll be beautified when switching to TextEditor

@aduth
Copy link
Member

aduth commented May 5, 2017

I mean more in the sense that we want post-content.js to serve as an example of what we'd expect the editor output to be.

@mcsf
Copy link
Contributor

mcsf commented Aug 23, 2019

  • We also ignore fallback blocks with empty content.

@youknowriad, this is quite a jump back in time 😅, but do you have any recollection of why this particular clause was added? I'm looking at this while reviewing #17164.

@youknowriad
Copy link
Contributor Author

@mcsf I think (I may be wrong though) that the idea is that if we serialize these blocks (don't ignore them) it will produce a lot of "empty lines" in the resulting HTML.

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.

Serialization: embrace whitespace
5 participants