Skip to content
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

[RN Mobile] Fix crash when merging block #15392

Merged
merged 29 commits into from
May 15, 2019
Merged

Conversation

Tug
Copy link
Contributor

@Tug Tug commented May 2, 2019

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

@nerrad
Copy link
Contributor

nerrad commented May 2, 2019

Hey just a couple friendly reminders:

  • For WIP it's more project friendly to use the draft pull feature in github because automatic codeowner and any requested reviewers will not get notified until the pull is published. So you can still work on it without worrying about increasing the notification noise to potential reviewers.
  • I started looking at this before noticing it was a WIP, on the surface it looks like you're not basing this branch off of a clean master because there seems to be a number of unrelated changes in the diff. You're likely already aware but just thought I'd point out in case you weren't. I see the force-push fixed 👍

@Tug Tug force-pushed the rnmobile/fix-crash-on-merge-2 branch from 6d06bcd to 9b93e9b Compare May 2, 2019 12:54
@Tug Tug removed request for mkaz and karmatosed May 2, 2019 13:05
@@ -343,6 +391,8 @@ export class RichText extends Component {
if ( onRemove && empty && isReverse ) {
onRemove( ! isReverse );
}

event.preventDefault();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this make sense on RN?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@hypest hypest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor Author

@Tug Tug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as well!

@etoledom etoledom self-requested a review May 15, 2019 10:29
Copy link
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants