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

Parser: Add "void" block signature #1134

Merged
merged 3 commits into from
Jun 12, 2017
Merged

Conversation

dmsnell
Copy link
Member

@dmsnell dmsnell commented Jun 11, 2017

Resolves #1085

Sometimes a block doesn't need anything inside of the block comment tags
as children. One example of this is the traditional "more tag," which
only needs to indicate position.

These changes introduce a new syntax element, the "void block,"
borrowing from the HTML5 nomenclature for HTML void elements.

In this new element, there are no children.

Since nothing changes on the JavaScript size this could be considered an
optimization whereby any block with no children serializes to a
self-closing block comment tag, saving some space and improving
readability of the post_content.

@dmsnell dmsnell added the [Feature] Parsing Related to efforts to improving the parsing of a string of data and converting it into a different f label Jun 11, 2017
@westonruter
Copy link
Member

@dmsnell is this not fixing #1085?

@mtias
Copy link
Member

mtias commented Jun 11, 2017

I like this for the terseness, thanks for the quick addition.

@nylen
Copy link
Member

nylen commented Jun 12, 2017

The syntax and code seem fine 👍

This PR should also update the serializer to use this format if a block has no content, and one or more tests should be added.

Sometimes a block doesn't need anything inside of the block comment tags
as children. One example of this is the traditional "more tag," which
only needs to indicate position.

These changes introduce a new syntax element, the "void block,"
borrowing from the HTML5 nomenclature for HTML void elements.

In this new element, there are no children.

Since nothing changes on the JavaScript size this could be considered an
optimization whereby any block with no children serializes to a
self-closing block comment tag, saving some space and improving
readability of the `post_content`.
@dmsnell dmsnell force-pushed the parser/add-void-block-signature branch from a9334ad to af617a2 Compare June 12, 2017 18:48
@dmsnell
Copy link
Member Author

dmsnell commented Jun 12, 2017

I'm having real trouble here making the tests work for this and I'm also experiencing an odd problem on save.

Problem with tests

  • The tests in full-content.js are hard-coded to a behavior I don't understand. it looks like those tests are supposed to be snapshot tests but they also appear to be testing a few different types of behaviors and I can't figure out what. there's quite a bit of programming in that test file which is making it hard for me to understand the goals.

Problem with save behavior

  • We seem to be exhibiting a behavior on save where if we don't have a block of a specific type registered that it will transform that block into a freeform block. This isn't good because the block could be a valid block and we may just happen to not have the right plugin installed. In the editor view I can understand treating it as freeform (though that's probably wrong too since it's a valid block) but we shouldn't be mangling it on save. I think this is an error not a decision, or at least I hope so.

Not sure how to move forward with this until I can get some help on these tests. Manual testing seems to work as expected.

Also update the full-content fixtures with the resulting changes.
@nylen
Copy link
Member

nylen commented Jun 12, 2017

5ebe561 fixes the tests. This change really should have been part of #947 (and tests for the bugs fixed there should have been added there).

The intent and functionality of these tests are documented here.

Most of the test fixture changes necessitated by using the PEG parser look OK, one that I'm not sure about is adding \n as a child received by multi-paragraph text blocks.

@nylen nylen merged commit 06be7ca into master Jun 12, 2017
@nylen nylen deleted the parser/add-void-block-signature branch June 12, 2017 21:36
@dmsnell
Copy link
Member Author

dmsnell commented Jun 13, 2017

Thanks for the fixes and the link to the explanation @nylen!

This change really should have been part of #947

I'm not sure I follow why. Did the TinyMCE parser do this already? I hadn't realized it was a goal of the grammar before a few days ago.

Anyway, I am grateful for your help!

@nylen
Copy link
Member

nylen commented Jun 14, 2017

By "this change" I mean the specific change to use the PEG parser in the test suite as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Parsing Related to efforts to improving the parsing of a string of data and converting it into a different f
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants