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

Implement support for nested lists on GB-mobile. #15566

Merged
merged 13 commits into from
May 23, 2019

Conversation

SergioEstevao
Copy link
Contributor

@SergioEstevao SergioEstevao commented May 10, 2019

Description

This PR updates the RN list block code to support the following:

  • Activates the indent/outdent buttons on the UI for the list block
  • Improves the handling of the Enter character and make sure the format of text between Aztec and the format library is consistent
  • Makes the handling of delete of new lines to be handled by the rich text format structure, instead of the native side.

How has this been tested?

This can be tested using this PR on the GB-mobile repo.

Screenshots

Simulator Screen Shot - iPhone Xʀ - 2019-05-10 at 17 43 25

Types of changes

New feature and bug fix on nested lists.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@SergioEstevao SergioEstevao added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label May 10, 2019
Copy link
Contributor

@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.

Great work here @SergioEstevao

I tested both on a real android device and with my computer keyboard on an emulator and tried to reproduce the issues described in wordpress-mobile/gutenberg-mobile#874 but could not 🎉

However I noticed a bug when splitting and merging list blocks, not sure it's related to your change yet. Will investigate.

@Tug
Copy link
Contributor

Tug commented May 17, 2019

Here is a recording show off the issue:

output

@SergioEstevao
Copy link
Contributor Author

SergioEstevao commented May 17, 2019 via email

onFormatChangeForceChild( record ) {
this.onFormatChange( record, true );
}

onFormatChange( record ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Tug should we rename this to onChange to match the web side?

Copy link
Contributor

@Tug Tug May 17, 2019

Choose a reason for hiding this comment

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

We might try now! But I think it's outside the scope of this PR...

On the web onChange accepts a record and is going to apply it to the DOM tree to apply only the required modifications.
Since we don't use the DOM on native, we created our own versions of onChange, one for value change (we kept the name onChange but we accept a native event argument instead) and one for format change (we called onFormatChange) in which we're calling valueToFormat to convert the record back into html to update the value instead of the dom.

@SergioEstevao
Copy link
Contributor Author

SergioEstevao commented May 17, 2019 via email

@SergioEstevao
Copy link
Contributor Author

@daniloercoli can you help on the Android side?

@etoledom
Copy link
Contributor

etoledom commented May 20, 2019

@SergioEstevao @Tug - I was able to reproduce #15566 (comment) in Android, iOS, and Web. So I'd say it's a bug on the web side.

web-list

EDIT:
Added a more complete review here: wordpress-mobile/gutenberg-mobile#991 (review)

@daniloercoli
Copy link
Contributor

Did you try on iOS on pressing backspace on a multi-line para block created on the web?

@SergioEstevao
Copy link
Contributor Author

Did you try on iOS on pressing backspace on a multi-line para block created on the web?

I went ahead and try it and it work fines on iOS, because the onDelete event is not sent to the JS side and it's still being processed in Aztec.

For the record on iOS the <br> element is translated to a lineSeparator character that is no intercepted as end of paragraph. So it not triggers a onBackspace event when deleting around it.

@SergioEstevao
Copy link
Contributor Author

@daniloercoli update with the latest master can you please give it a look?

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.

5 participants