-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Add quote↔text block switching; add block switcher validation #465
Conversation
blocks/library/quote/index.js
Outdated
transform: ( { value, citation } ) => { | ||
if ( citation ) { | ||
return new Error( | ||
'Quote citation would be lost on transform.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should just create two paragraphs instead of erroring here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love this because switching a quote block to a text block and back would leave the citation as a separate paragraph.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a problem for me, because it's an explicit change asked by the user (maybe discuss in the comment bellow)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is an acceptable data loss. I think the razor we should hold any transformation against is that it has to be non-destructive, which is fuzzy enough that it would okay this transformation, but disallow transforming it into a gallery for example (which would make no sense). In other words, yes you might lose some formatting, but the data is still there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I don't have a big problem with this specific case, I'll change it to include the citation as a separate paragraph.
const attributes = transformation.transform( block.attributes ); | ||
if ( attributes instanceof Error ) { | ||
// Blocks can perform validation and cancel transformations if needed. | ||
return attributes; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning null
above is also an error (no transformation found), we should be consistent.
In the same time, I'm wondering if we really need to throw specific errors. I don't see any use case where we'd want to perform different actions depending on the error. Should we show this error somewhere in the UI? I don't think so, it's not relevant for the user I think.
Note that the transforms
API is not used for block switching only. If we introduce new returns values like these, we'd want to handle them in all "clients" see #457
Also, I think we should perform the transformation even if we have data loss (we should limit the data loss) because the user explicitly asks for the transformation, so it's not a surprise for him to get its content changed a bit. For example when switching from a text block to a heading block, I'm not agains concatening with spaces the different paragraphs. If we reject the transformation, this means backspacing from a text block to a heading won't work either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would you suggest returning instead of null
? To me this makes sense because it is a generic failure case which doesn't have specific meaning to the user (something went wrong in the code).
I'll address the rest of your comments below.
By saying this, we are setting an expectation for our users that they understand what a block switch entails internally. I think this isn't a reasonable expectation, and it also isn't reasonable to me to have UI controls that delete data or formatting without a warning or explanation.
The idea here is that we disable the switch until we can verify that there is no data loss, and give a specific reason for it being disabled in the UI so that the user can fix it. Currently this is a
I'd have to say I am against this. Transforming a multi-paragraph text block into a heading block isn't an operation that makes much sense to me.
I also think this should only work if the text block contains one paragraph. Either that, or we just make the new block a text block. |
I think there's some confusion among all of us that we need to settle. There are a number of tickets around contiguous text elements.
In addition to this it's a little unclear exactly what contiguous blocks we merge. Is it just paragraphs following each other, or is it heading, paragraph, list, quote also? Are the above tickets all the same? The answers might dictate a UI for it. I made some mockups here which I That is — even though behind the scenes we are technically looking at one big continuous text block, the text elements inside still have a traditional looking block UI. So you'd still select a single paragraph as a block, and be able to transform that to a heading. If you selected across these paragraphs, you'd not be shown the switcher. |
It feels like there should be a simpler way to solve this.
To me this (and your UI mockups) says that 1 paragraph should be 1 block. If we do that instead, the need for the validation / rejection logic in this PR probably also goes away.
If we do create a "paragraph" block, then under our current implementation this and a whole bunch of other related flows (#179) become impossible, which is why in #409 I advocated for a text block containing multiple paragraphs. After seeing the resulting design and technical complexity, I'm starting to question that decision. |
You're right, it does seem complex.
The idea that Text is a single block, with a single set of controls docked to the top of it, appeals to me. It will also provide some welcome UI similarities to the Freeform block (a.k.a. TinyMCE block), which will be mostly the same except perhaps with a few more features. |
As a small update on this, we should explore two approaches for writing and wrangling text in the editor:
We should build both, see which one works best, and keep only the one that works best. There is a mockup here, of how the continuous text block could look. In that mockup, quotes and lists are part of the toolbar. Selecting one of those options after having selected one or multiple paragraphs could either split the block when the selection is converted, or we could decide that quotes and lists are part of the continuous text block. See also #447. |
This is outdated, see #484 instead |
This PR adds the ability to switch between
quote
andtext
blocks, and also adds a mechanism for blocks to reject transformations.Currently the following two transformations are rejected because they would cause data loss:
In the UI, we show this situation by disabling buttons in the block switcher:
It would be nice if we had a
Tooltip
component to use instead of atitle
attribute, and perhaps a little warning icon, but I think this is good enough for a first PR.