-
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
Fix undo on paragraph split #6964
Conversation
d069e99
to
5f16dca
Compare
Seeing an error on splitting:
Otherwise I think this is looking nice. |
editor/store/actions.js
Outdated
* | ||
* @return {Object} Action object. | ||
*/ | ||
export function replaceBlocks( uids, blocks ) { | ||
export function replaceBlocks( uids, blocks, blockToSelectUid ) { |
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.
This looks good to me but would love to know what @youknowriad thinks. :)
) ); | ||
replaceBlocks( [ ownProps.uid ], blocks ); | ||
replaceBlocks( [ ownProps.uid ], blocks, blockToSelectUid ); |
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.
Why don't we just call selectBlock
or something like that right after? It seems weird to tie the replacement of blocks with selection?
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.
Now the action object contains the property selectBlock, unfortunately, I was not able to use it in the parameter name because we already have a function with that name.
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 you misunderstood my remark here. For me we shouldn't add this new parameter here and just call another action right after selectBlock( blockToSelectUid )
.
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.
Hi @youknowriad I missed your suggestion at first but now I updated the code to follow it :)
editor/store/reducer.js
Outdated
// as the next selected block. If empty replacement, reset to null. | ||
const nextSelectedBlockUID = action.blockToSelectUid ? | ||
action.blockToSelectUid : | ||
get( action.blocks, [ 0, 'uid' ], null ); | ||
|
||
return { | ||
...state, |
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 know it was already the case before but I feel the REPLACE_BLOCKS
action should not touch the selection at all unless the currently selected block is being replaced.
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.
Hi @youknowriad, that's exactly the current behavior. We have a condition at the start that guarantees that if the currently selected block is not replaced we don't change the selection:
if ( action.uids.indexOf( state.start ) === -1 ) {
return state;
}
@@ -236,14 +236,12 @@ class ParagraphBlock extends Component { | |||
if ( after ) { | |||
blocks.push( createBlock( name, { content: after } ) ); | |||
} | |||
const replaceArrayStart = before ? | |||
[ createBlock( name, { ...attributes, content: before } ) ] : | |||
[]; |
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.
There's a subtle difference here with the previous behavior. We're creating a new block with a new UID which resets all other properties of the blocks and cause a remount instead of a rerender. Maybe it's fine, just noting.
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.
That's true we change the uid and we have a remount but I think doing that is ok.
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.
Could we reuse the block that is being replaced in the reducer if the name is the same?
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.
We had already an onReplace in the paragraph with logic to reuse attribute when replacing with the same block I added one line there that also reuses the UID. I think it simplifies the things and removes the need for any TinyMCE hacks.
5f16dca
to
7cc2438
Compare
Hi @iseulde,
I added some checks that should avoid this error, thank you for catching it. @iseulde @youknowriad, thank you for the reviews the feedback was addressed. |
7cc2438
to
0004562
Compare
editor/components/rich-text/index.js
Outdated
@@ -685,6 +689,9 @@ export class RichText extends Component { | |||
rect = getRectangleFromRange( this.editor.selection.getRng() ); | |||
} | |||
const focusPosition = this.getFocusPosition( rect ); | |||
if ( ! focusPosition ) { |
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.
When would this fail?
editor/components/rich-text/index.js
Outdated
@@ -426,6 +426,10 @@ export class RichText extends Component { | |||
* @return {{top: number, left: number}} The desired position of the toolbar. | |||
*/ | |||
getFocusPosition( position ) { | |||
// if the node was removed or replaced return immediately. | |||
if ( ! this.containerRef.current ) { |
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'm surprised this even happens. Maybe worth finding out the root cause of this and comment. It might even be the same as #6981.
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 was not related to #6981 I added it was a patch and it did not fix the problem here. But now that we reuse the UID this problem does not happen and all the changes to TinyMCE here reverted.
@jorgefilipecosta Have you tried this? #6964 (comment) I think that would make the code a bit cleaner too. :) |
0004562
to
9272fd5
Compare
@@ -84,7 +84,7 @@ class ParagraphBlock extends Component { | |||
this.setFontSize = this.setFontSize.bind( this ); | |||
} | |||
|
|||
onReplace( blocks ) { | |||
onReplace( blocks, blockToSelect ) { |
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 see this change only being applied to the paragraph block. Should it be generalised to all blocks that have an onSplit
prop, such as headings?
Is this still desired to be continued? Is it better represented in the alternative proposal discussed in #8119 ? |
Closing this in favor of #8119. |
Description
Splitting was not a single operation but two operations insert blocks + setAttributes. That made undo behave in a strange way.
In this PR we make onSplit a single operation using onReplace (props to @iseulde for this idea).
onReplace by default selects the first block. That was not what we wanted in this use case so a new prop was added that allows choosing the bock we want to select.
To accomplish this onReplace was changed to not clone the blocks because cloning changes the UID.
I think we should remove clone from onReplace anyway even if we find an alternative approach to the PR. When a user asks for some blocks to be replaced I think the expectation is that the new blocks the user set are the used ones and the UID's saved in the state are the same that the user passed. In PR #6325 I felt the same need and I applied the same change to onReplace.
We did not added more test cases yet if we find this approach something we want to pursue I will add them.
How has this been tested?
Add a paragraph write some text, press enter in the middle, see the paragraphs were divided. Press undo see that undo works as expected and the paragraphs are joined again.
Try to do some testing to verify removing clone fro onReplace does not cause unexpected behaviors.
Screenshots
Before:
After: