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

Try: Improve round-trip transformation of text and quote blocks #1774

Closed
wants to merge 1 commit into from

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Jul 7, 2017

With this PR, given a quote block:

image

Converting it to a text block yields:

image

This introduces a <br class="citation-break"> element to delineate the original quotation from the citation so it can be converted back into a quote:

image

There are two opening p tags there. So this is one known issue with the patch. It generates this in the console:

Warning: validateDOMNesting(...): <p> cannot appear as a descendant of <p>.

Fixes #1765.

@youknowriad
Copy link
Contributor

Sorry forgot about this PR :( I should have reviewed a long time ago.

These transforms changed master, a rebase is necessary right now.

@gziolo
Copy link
Member

gziolo commented Jan 27, 2018

@iseulde or @mcsf is this something we still need to include? Can we help to merge that one?

@gziolo gziolo added the [Feature] Blocks Overall functionality of blocks label Jan 27, 2018
@mcsf
Copy link
Contributor

mcsf commented Jan 27, 2018

Hey, @westonruter. Having a look at this now. I disagree with the proposed solution in that:

  • I don't believe we should try to port the intricacies of one block to the one we are transforming to. Sometimes these operations will be destructive—typically when the source block encodes more essential state—and that's fine.

  • Arguably, this proposal seeks to make Paragraph blocks conform to a certain shape they weren't intended for. Not that line break isn't supported—it is—but here we are furthermore injecting an invisible mark, effectively entangling user content with implementation details of the editor and the transform stack.

  • A merged PR, Fixed quote paragraph transformations. #3802, opens the precedent for some smarter transforms involving quotes. In it, transforming a list block with items A, B, C will yield the quote with text A & B and citation C. Now that multi-block transforms are supported (Add multi-block transform functionality. #3357), we could consider transforming a sequence of paragraphs into a quote with citation, and thus the opposite transform—from quote to paragraphs—would just need to output two blocks, one for each field.

  • In parallel, we could look into smarter optimizations for the benefit of the user that don't require encoding application state within the block. For instance, if a type switch is requested from paragraph to quote, Gutenberg can look at whether the paragraph had initially been converted from a quote; if so, and if it hasn't been touched by the user, we can just use the undo buffer to recover the original quote, with no information loss. This would have to be made in a simple way, relying on the existing history subsystem and avoiding adding overhead.

I recommend closing this PR and exploring alternatives.

@gziolo gziolo removed their request for review January 27, 2018 17:21
@gziolo gziolo added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Jan 27, 2018
@youknowriad
Copy link
Contributor

Thanks for working on this, I'm closing per @mcsf comments. Let's try alternatives.

@gziolo gziolo deleted the fix/text-quote-transforms branch May 7, 2018 11:14
Tug pushed a commit that referenced this pull request Jan 16, 2020
[RNMobile] Shortcode block support
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 [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants