Skip to content

Conversation

@planarvoid
Copy link
Contributor

Fix

The nested lists were not previously working properly. In this PR I'm fixing the behaviour.

  • the clicked nested list is now correctly highlighted in the toolbar
  • you can change type of the selected nested list (without changing the parent list type)
  • you can change type only of the parent list type, not of the child
  • you can change type of both if you select both

Test

  1. Call toolbar.setToolbarItems(ToolbarItems.BasicLayout(ToolbarAction.LIST, ToolbarAction.INDENT, ToolbarAction.OUTDENT)) in the MainActivity to enable indents and list operations
  2. Run the app
  3. Add a list
  4. Try toggling its type
  5. Add nested lists by indent
  6. Try toggling type of the nested list
  7. Try toggling type of the parent list
  8. Click around and notice the toolbar list icon is highlighted according to the selected list

Review

@AmandaRiu

Make sure strings will be translated:

  • If there are new strings that have to be translated, I have added them to the client's strings.xml as a part of the integration PR.
improved-lists.mp4

@planarvoid planarvoid added the bug label Apr 15, 2022
@planarvoid planarvoid requested a review from AmandaRiu April 15, 2022 09:02
@planarvoid planarvoid self-assigned this Apr 15, 2022
Copy link
Contributor

@AmandaRiu AmandaRiu left a comment

Choose a reason for hiding this comment

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

@planarvoid Changes look good and list manipulation definitely seems better, but I did run into funkiness with the enabling/disabling of the indent/outdent buttons:

list-button-enable-disable.mp4

As well as with highlighting list items across indentations and attempting to indent further:

list-highlight.mp4

And some items losing their list status:

list-issue-2.mp4

Base automatically changed from feature/item-indent to trunk April 20, 2022 20:35
@planarvoid
Copy link
Contributor Author

Thanks for the review @AmandaRiu, all items were great catches! I have fixed all of them in the last 3 commits and I think everything works well

@AmandaRiu AmandaRiu self-requested a review May 10, 2022 23:18
Copy link
Contributor

@AmandaRiu AmandaRiu left a comment

Choose a reason for hiding this comment

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

What an incredible improvement @planarvoid ! I tested a bunch and only found one issue, a scenario that leads to an ANR. I was able to replicate the ANR with the following steps:

  1. Create an ordered list that looks like this:

Screen Shot 2022-05-10 at 10 19 40 PM

  1. place cursor in "Four" and tap the button to outdent that item so the three middle list items are all at the same indentation.
  2. Select items "three", "four" and "five" and tap the button to outdent them so that all list items will be in the same level. This causes the app to freeze and eventually an ANR will display.

Here's a video:

screencapture-1652235250473.mp4

AmandaRiu and others added 2 commits May 10, 2022 22:38
@planarvoid
Copy link
Contributor Author

Good catch @AmandaRiu ! It was caused by a solution I've used previously to determine the following list item (to find out the desired behaviour of the outdent). I've fixed it in 8f1f87c

@planarvoid planarvoid requested a review from AmandaRiu May 12, 2022 10:44
Copy link
Contributor

@AmandaRiu AmandaRiu left a comment

Choose a reason for hiding this comment

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

Works beautifully!!! Very nice work @planarvoid !!! 🥳

:shipit:

@AmandaRiu AmandaRiu merged commit 4b69fe1 into trunk May 12, 2022
@AmandaRiu AmandaRiu deleted the feature/enable-combined-lists branch May 12, 2022 23:16
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.

3 participants