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

Nested lists #991

Merged
merged 15 commits into from
May 23, 2019
Merged

Nested lists #991

merged 15 commits into from
May 23, 2019

Conversation

SergioEstevao
Copy link
Contributor

Fixes: #874

Related GB PR: WordPress/gutenberg#15566
Relate Aztec PR: wordpress-mobile/AztecEditor-iOS#1181

This PR updates the list code to support the following:

  • Activates the indent/outdent buttons on the UI for the list block
  • Makes the handling of delete of new lines to be handled by the rich text format structure, instead of the native side.

How to test:

  • First of all try, the issue stated on List block split problem with nested lists #874 to see if it's working correctly
  • Then try to use the indent and outdent buttons and see if they work correctly
  • Try to delete list items and see if it works correctly

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.

Hey @SergioEstevao - Tested on iOS and it's looking good 🎉

The issue seems to be completely fixed ✅


  • Then try to use the indent and outdent buttons and see if they work correctly

This one is working mostly fine. I did find a small issue (in both iOS and Android):

  • Create a list with a couple of indented items at the end.
  • Select another block.
  • Select again the list with the cursor in the last item
  • Press "de-indent" (<-) to try to remove the indentation of the last item.
  • It won't work.
    list-item-02

  • Try to delete list items and see if it works correctly

This one also works good! ✅

There is an issue spotted by @Tug (WordPress/gutenberg#15566 (comment)) but it seems to be a web issue (or expected behavior?) WordPress/gutenberg#15566 (comment)

I found another issue that is reproducible in the web side:

  • Copy this test html and paste it in gutenberg-mobile:
<!-- wp:list -->
<ul><li>line 1<ul><li>line 21</li><li>line 22<ul><li>line 31</li><li>line 32</li></ul></li></ul></li><li>line 3</li><li>line 4</li><li>line 5</li></ul>
<!-- /wp:list -->
  • Set the caret at the end of the last item
  • Press enter to break out of the list block
  • In the new paragraph block, press backspace to merge.
  • Check what happened to the indented list items from above (they all merged in one).

lists-03


From all these issues, only the first one described seems to be related to only gutenberg-mobile.

# Conflicts:
#	gutenberg
#	react-native-aztec/ios/Cartfile
#	react-native-aztec/ios/Cartfile.resolved
@SergioEstevao
Copy link
Contributor Author

@etoledom and @daniloercoli merged latest develop on this PR.

Do you mind to give it another look?

@daniloercoli daniloercoli self-requested a review May 23, 2019 07:42
Copy link
Contributor

@daniloercoli daniloercoli left a comment

Choose a reason for hiding this comment

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

As discussed in chat convo this PR is in a good shape, and working mostly fine on Android also.
We noticed the problem with the backspace that doesn't "outdent" the item in nested listed, instead it merges the current item with the one above.
Not a big problem anyway, and we will tackle it in a following PR. cc @mkevins

Btw, while testing this "hardly" I got a crash with the caret set out of bounds. Was not able to reproduce the issue in following tests, so it may be a rare case, but worth to take an eye on it.

LGTM!

@etoledom
Copy link
Contributor

From all these issues, only the first one described seems to be related to only gutenberg-mobile.

Tested this and it seems to be fixed ✅

@etoledom
Copy link
Contributor

etoledom commented May 23, 2019

A heads up:
Unit Tests should pass after merging #1008

Edit: Not sure about tests on Device.

@SergioEstevao SergioEstevao merged commit d770b77 into develop May 23, 2019
@SergioEstevao SergioEstevao deleted the issue/874_nested_lists branch May 23, 2019 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

List block split problem with nested lists
3 participants