[RN Mobile] Fix crash when merging block #15392
Conversation
Tug
requested review from
adamsilverstein,
aduth,
ajitbohra,
atimmer,
chrisvanpatten,
dmsnell,
ellatrix,
gziolo,
jorgefilipecosta,
karmatosed,
mapk,
mkaz,
mmtr,
nerrad,
noisysocks,
oandregal,
notnownikki,
ntwb,
Soean,
swissspidy,
talldan and
youknowriad
as code owners
|
Hey just a couple friendly reminders:
|
Tug
force-pushed
the
rnmobile/fix-crash-on-merge-2
branch
from
6d06bcd to
9b93e9b
Compare
|
FYI, still working on some issue fixes (pushed some WIP commits to Edit on May 13th 2019: Added couple more fixes, including an AztecAndroid change for avoiding some stray onSelectionChange events. |
| // /// | ||
| // } else { | ||
| // if ( doUpdateChild ) { | ||
| // this.lastEventCount = undefined; |
There was a problem hiding this comment.
@hypest - This is the source of the format issue on iOS. If eventCount is other than undefined, the Aztec-iOS wrapper will not update its content, so we need to undefine it at some point, for the format change to work.
There was a problem hiding this comment.
I'm not sure what was the reason to do this change, but if it's working properly on Android, maybe we can modify the native iOS Aztec wrapper to work as the Android one does?
There was a problem hiding this comment.
Chatted over Slack and I think the culprit is to be found in the shouldComponentUpdate function? The assumption we can try to align to is that a "normal" prop change (initiated by https://github.com/WordPress/gutenberg/pull/15392/files/380660e7d751834ac0de4eb7f207d0c0f89fa863#diff-4828a21853e899e5a36faecfa96d83e8R272) should be enough to cause the code to try and force the changes to the Aztecs.
Pressing on the buttons didn't change the format of the selected text.
…/fix-crash-on-merge-2
| @@ -343,6 +391,8 @@ export class RichText extends Component { | |||
| if ( onRemove && empty && isReverse ) { | |||
| onRemove( ! isReverse ); | |||
| } | |||
|
|
|||
| event.preventDefault(); | |||
There was a problem hiding this comment.
Does this make sense on RN?
There was a problem hiding this comment.
Not really indeed.
I don't think it's doing anything at the moment (it probably could if we handled it on the native side?), I just added it because I wanted the code to get closer to the RichText web version so we can merge the two in the short term.
If you find it confusing I don't mind removing it.
There was a problem hiding this comment.
It's ok for me, it will actually be nice that this sent something back to the native side, and then we could decide how to handle the Enter/Delete after JS processed the event.
…es won't change the format of the selected text
Tug
added
Mobile App - i.e. Android or iOS
This PR aims to upgrade RichText on mobile native to use the latest changes from the web, mainly those made in #14640
It also removes the customization we made to the rich-text library since writing in multiple formats at once is now stable on the web.
Testing Instructions
Tested with wordpress-mobile/gutenberg-mobile#949