-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
RichText: stabilize onSplit #14765
RichText: stabilize onSplit #14765
Conversation
cbf86be
to
0a85d5c
Compare
0a85d5c
to
ddc3533
Compare
ddc3533
to
e1e6b68
Compare
d936b26
to
0008d0e
Compare
3b721c7
to
7eca736
Compare
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.
Left some few comments, but I'm going to take this for a spin now.
*/ | ||
splitContent( blocks = [], context = {} ) { | ||
if ( ! this.onSplit ) { | ||
onSplit( record, pastedBlocks = [] ) { |
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 it would be good to clarify the different use cases this function is called (sorry If it's somewhere else and I missed it), something like:
- putting the caret in the middle of a RichText and pasting
- click "Enter" in the middle of the text
- other use-cases I'm not aware of?
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.
Done in 199a1e9.
this.onSplit( before, after, ...blocks ); | ||
// If there are pasted blocks, set the selection to the last one. | ||
// Otherwise, set the selection to the second block. | ||
const indexToSelect = hasPastedBlocks ? blocks.length - 1 : 1; |
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 immediately clear to me why should we put the caret int the first block if there's no pasted blocks?
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.
1
means the second block. :) After pressing enter, the caret goes in the second block (there are only two)?
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.
Alternatively this could be written as blocks.length
I guess.
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.
Does this mean the index is always blocks.length - 1
(the last block)
splitContent( blocks = [], context = {} ) { | ||
if ( ! this.onSplit ) { | ||
onSplit( record, pastedBlocks = [] ) { | ||
const { onReplace, onSplit, onSplitMiddle } = this.props; |
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.
onSplitMiddle is not clear to me and I think it's not documented right?
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.
Is this something to be marked unstable?
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.
Seems good to mark as unstable for now. Will adjust.
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.
Done in 199a1e9.
b68e141
to
7af898f
Compare
@youknowriad I can't reproduce that issue. What browser are you using? |
@ellatrix I'm using Firefox |
@youknowriad It looks like it's a visual glitch that happens in |
@ellatrix I confirm, let's fix this bug separately. |
@youknowriad Addressed your feedback. Additionally, #14846 seems to fix the issue you're having in Firefox. |
Co-Authored-By: Riad Benguella <[email protected]>
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.
Great work
Thanks @youknowriad! |
Description
Stablizes the
RichText
onSplit prop.Additionally it fixes issues where core blocks (heading and list) do not take into account any null values passed to
unstableOnSplit
.How has this been tested?
Screenshots
Types of changes
Checklist: