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

Pullquote: Fix parsing/serializing multi-paragraphs in pullquotes #1987

Merged
merged 5 commits into from
Jul 27, 2017

Conversation

youknowriad
Copy link
Contributor

closes #1982

This PR seeks to solve the parsing/serializing issues we were experiencing when typing multiple paragraphs in pullquotes.

Testing instructions

  • Create a multi-p pullquote
  • Check the markup in the text editor (no nested paragraphs
  • Save and refresh, the markup should stay the same

@youknowriad youknowriad added [Feature] Blocks Overall functionality of blocks [Type] Bug An existing feature does not function as intended labels Jul 24, 2017
@youknowriad youknowriad self-assigned this Jul 24, 2017
@youknowriad youknowriad requested review from mkaz and afercia July 24, 2017 14:08
Copy link
Contributor

@afercia afercia left a comment

Choose a reason for hiding this comment

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

@youknowriad tested a bit and seems this approach perfectly works in Gutenberg.

However, when editing a post with a pullquote in the Classic editor, and then again in Gutenberg, the content gets lost. Not sure if this should go in a separate issue.

For example: inserted a pullquote with 2 paragraphs:

<blockquote class="wp-block-pullquote alignnone">
    <p>My quote</p>
    <p>with multiple par</p>
    <footer>my caption</footer>
</blockquote>

Saved, then opened the same post using the Classic editor, switched to text mode and the markup was changed to:

<!-- wp:core/pullquote -->
<blockquote class="wp-block-pullquote alignnone">My quote

with multiple par

<footer>my caption</footer></blockquote>
<!-- /wp:core/pullquote -->

This perfectly works in the Classic editor, but when editing again in Gutenberg, it doesn't of course and the content gets stripped out.

Marking as "Request changes" just because I'm not sure if you want to open a new issue.

@nylen
Copy link
Member

nylen commented Jul 24, 2017

@afercia that is part of a much larger class of issues with the way we parse block content. Can you leave that comment on #1736 as an example?

@nylen nylen dismissed afercia’s stale review July 24, 2017 15:08

See above comment, this is a larger issue and should be handled separately.

@nylen
Copy link
Member

nylen commented Jul 24, 2017

0c3e5ac fixes the previously existing tests.

2f96b6c adds a new test for this condition. Strangely, before this PR, this test would have already passed - whatever issue caused the duplicate p tags was only appearing in actual usage, which is a bit concerning as it means there's some behavior we're not successfully testing.

Adding that new test surfaced a warning which I'm able to reproduce by using the editor:

  • Create a multi-paragraph pullquote block.
  • Switch from Visual to Text mode. During serialization, the following warning is generated:
Warning: Each child in an array or iterator should have a unique "key" prop.

Check the top-level render call using <blockquote>. See https://fb.me/react-warning-keys for more information.
    in p

@youknowriad
Copy link
Contributor Author

I fixed the warning, I'm not totally satisfied with the fix but it has to do with the Editable shape lacking keys. The Editable shape is part of a much larger concern.

@nylen
Copy link
Member

nylen commented Jul 25, 2017

There is an untested branch in the latest commit. How about fixing that by adding a <strong> or something in one of the paragraphs for the multi-paragraph test fixture?

I would like to know why this bug was occurring in the UI but not the test suite. Any thoughts there?

@youknowriad
Copy link
Contributor Author

I would like to know why this bug was occurring in the UI but not the test suite. Any thoughts there?

I thought about this and here is my hypothesis:

  • The parsing was dropping the p tag and leaving only the children
  • The serializer (save) was adding the p again

But when we use TinyMCE (the editable), for each item of the array, it adds a p wrapper if no wrapper is found (simple string) and when serialized, we end up with the double p.

@youknowriad
Copy link
Contributor Author

Added a strong to the multi-paragraph example. Is this ready to be merged?

Copy link
Member

@nylen nylen left a comment

Choose a reason for hiding this comment

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

🚢

<blockquote class="wp-block-pullquote alignnone">
<p>Paragraph <strong>one</strong></p>
<p>Paragraph two</p>
<footer>by whoever</footer>
Copy link
Member

Choose a reason for hiding this comment

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

whomever 🤓

@youknowriad youknowriad force-pushed the fix/pullquote-parse-serialize branch 2 times, most recently from 8cec198 to d64562e Compare July 27, 2017 09:38
@youknowriad youknowriad force-pushed the fix/pullquote-parse-serialize branch from d64562e to 11dc880 Compare July 27, 2017 09:49
@youknowriad youknowriad merged commit a41b185 into master Jul 27, 2017
@youknowriad youknowriad deleted the fix/pullquote-parse-serialize branch July 27, 2017 09:58
Tug pushed a commit that referenced this pull request Mar 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The pullquote block outputs nested paragraphs
4 participants