[RNMobile][Android] Fix jumping keyboard when adding a new paragraph using "return" key #28101
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +66.3 kB (+5%) 🔍 Total Size: 1.37 MB
ℹ️ View Unchanged
|
dratwas
requested review from
ajitbohra,
cameronvoell,
ellatrix,
guarani and
talldan
as code owners
dratwas
requested review from
youknowriad
and removed request for
talldan,
ajitbohra and
guarani
youknowriad
reviewed
Jan 20, 2021
There was a problem hiding this comment.
While I like the fact that when splitting, the original block should keep its id, I'm not sure I like how it's done here. I'd love @ellatrix's input on this PR.
|
Hey, @youknowriad could you please take a look one more time? I have changed the way I return the "original" block from |
youknowriad
reviewed
Jan 25, 2021
| @@ -61,7 +69,17 @@ function HeadingEdit( { | |||
| value={ content } | |||
| onChange={ ( value ) => setAttributes( { content: value } ) } | |||
| onMerge={ mergeBlocks } | |||
| onSplit={ ( value ) => { | |||
| onSplit={ ( value, keepId ) => { | |||
| if ( keepId ) { | |||
There was a problem hiding this comment.
Do we really want to have the "keepId" argument? I mean why not just replace the createBlock( 'core/heading' call a few lines down here?
There was a problem hiding this comment.
Yes, we still need the keepId since it determines if we should use the original block or create a new one. W/o that we wouldn't know if we should create a new block or make changes in the "original" one. I am wondering if I could use getBlock in the rich-text component and use onSplit only for creating new blocks but I'm not sure if it's a good way because we would lose the "flexibility" in that mechanism (currently it could work differently for heading, paragraph, list etc).
There was a problem hiding this comment.
Adding keepId is a horrible API. We should go for one or the other for all cases. Any alternatives we can explore here?
There was a problem hiding this comment.
Adding keepId is a horrible API. We should go for one or the other for all cases.
Can you elaborate a bit? :)
IMHO it is still better than removing the original block and replacing it. Even from the performance perspective update is faster than unmount + mount (we use the clientId as a key in our block-list).
Any alternatives we can explore here?
As I mentioned I was wondering if it could be done inside rich-text/index.js. We could get block and change attributes inside instead of calling onSplit for the original block but i haven't investigated it.
youknowriad
reviewed
Jan 25, 2021
| content: value || '', | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Same question as above
There was a problem hiding this comment.
Should we do the same for the web version of this file?
There was a problem hiding this comment.
Yes, we still need the keepId, unless you have a different concept for it.
I already added it to the web version :)
youknowriad
reviewed
Jan 25, 2021
|
Why is the keyboard jumping? Because the original paragraph is removed? The reason we're creating two new paragraphs is because one split action should be one change in the store (and so one undo level for the user). The way we achieve that right now is by replacing the current paragraph with two new ones. Alternatively we could create a new action for the store to split a block in one go. I don't like the current approach of having two different behaviours for web and mobile at all. This is just making our APIs more complex and harder to use. |
Exactly. The original paragraph is removed so that means the selected block is unmounted, the
This still works in the same way. We replace the original paragraph with two others but the "first" one has the same clientID as the original one. Thanks to that the original block is not unmounted and mounted but only updated. It is still done in one
Hmmm, there is no different behavior for web and mobile. This works the same for mobile and web. The heading block has shared code so it has to be the same and I also updated the |
|
Why do we need the keep ID parameter then? Can't we always keep ID? |
We can not because the |
|
I created a PR to handle keeping the ID #28505 |
youknowriad
deleted the
rnmobile/rich-text/jumping-keyboard-when-adding-new
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
After merging of #27737 the issue with the jumping keyboard was introduced. When splitting a paragraph or adding a new one by using "return" key the keyboard jumps because the paragraph that is selected is unmounted and the new one is created. (we call blur in
componentWillUnmount)I think that we don't need to create a new paragraph (or others block but I started from a paragraph) for the text that is before the caret. We could use the same block and set the correct attribute to it, or create a new one but with the same
clientId. Thanks to that we will avoid unnecessary unmounting and mounting of the paragraph that is selected.Outdated
Unfortunately, I couldn't find an option to create or clone blocks with a specific clientID. In this PR I added a possibility to set clientID to the createBlock. I had also added one argument to the `onSplit` to determine if the returned block should have the same clientID as the selected block.I'm not sure if it is the correct way to do that and if someone knows how to achieve that in another, more elegant way, please let me know.
In this PR I use the original block but with different
attributes.contentdepending on the passed value inonSplitfunction.This fixes also #26252
How has this been tested?
Screenshots
Types of changes
Checklist: