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

[Rnmobile] Fix the list handling on Android #15168

Merged
merged 14 commits into from May 2, 2019

Conversation

Projects
None yet
4 participants
@hypest
Copy link
Contributor

commented Apr 25, 2019

Description

Introduces changes to how the the React Native mobile app's RichText handles the events coming from the native side, especially for Android.

How has this been tested?

Using the gutenberg-mobile PR: wordpress-mobile/gutenberg-mobile#928

Types of changes

  1. Introduce an extra set of flags to denote when a change has originated from Aztec (and thus the RN app should not try to force it back to Aztec) and whether the "Enter" key event was emitted before or after the text changed and was processed by Aztec. If after the change, the RN app should only try to update its internal representation in an attempt to mirror Aztec's. If before the change, it's OK for the RN app to process the event and send back to Aztec any text changes that need to happen.
  2. Default onFormatChange() to update Aztec. Can (and is) overridden by using the firedAfterTextChanged flag to avoid updating Aztec if the change has already happened in Aztec.
  3. Set deleteEnter on the paragraph block's RichText component to denote that we want Aztec to delete the Enter key at the end of the block, otherwise Aztec-Android leaves it there. We don't need to specify it for the list block though because Aztec removes the Enter there as part of its list handling process.

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.

hypest added some commits Apr 23, 2019

Use a flag to signal Aztec-originated changes
And assume that when that flag is false, component changes need to get
sent/reflected down to Aztec.
Differentiate Android and iOS since assumptions diverged
The iOS side still expects to just check against `this.lastContent` to
force the change into Aztec.

@hypest hypest added the [Block] List label Apr 25, 2019

@daniloercoli daniloercoli self-requested a review Apr 26, 2019

@hypest hypest marked this pull request as ready for review Apr 30, 2019

hypest added some commits Apr 30, 2019

Have Aztec delete the detected Enter key for paragraphs
Aztec-Android doesn't swallow the Enter key (like the list handling does) so,
instruct Aztec to delete it for the paragraph block.
Don't force Aztec update on format button toggles
Doing this by reverting onFormatChange's behavior back to assuming
doUpdateChild is false by default.
@daniloercoli
Copy link
Contributor

left a comment

We've had a chat convo about the possibility to get rid of the newly introduced deleteEnter prop, but agreed it's the best solution at the moment.
We will probably need to revise the code once again, when will introduce multi-line paragraph blocks (we will need to handle the enter.key detection again).

@hypest

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

Will merge this PR now and will cherry-pick the merge-commit into our special branch (rnmobile/master-fork-before-richtext-selection-state-change) we'll use for tomorrow's code freeze.

@hypest hypest merged commit 9a99071 into master May 2, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@hypest hypest deleted the rnmobile/fix-list-handling-android branch May 2, 2019

hypest added a commit that referenced this pull request May 2, 2019

[Rnmobile] Fix the list handling on Android (#15168)
* If text already changed, don't modify it

* Able to not lose content

* Use a flag to signal Aztec-originated changes

And assume that when that flag is false, component changes need to get
sent/reflected down to Aztec.

* Differentiate Android and iOS since assumptions diverged

The iOS side still expects to just check against `this.lastContent` to
force the change into Aztec.

* Force Aztec update if "Enter" fired before text change

* Need to specify firedAfterTextChanged on all Aztec events

* Fix lint issues

* chore: Fix: Lint error that makes unit tests (and CI tests) fail. (#15073)

* Trivial change to trigger Travis

* Revert "Trivial change to trigger Travis"

This reverts commit e22ffde.

* Just use onFormatChange which now defaults to "force"

* Have Aztec delete the detected Enter key for paragraphs

Aztec-Android doesn't swallow the Enter key (like the list handling does) so,
instruct Aztec to delete it for the paragraph block.

* Don't force Aztec update on format button toggles

Doing this by reverting onFormatChange's behavior back to assuming
doUpdateChild is false by default.

@youknowriad youknowriad added this to the 5.7 (Gutenberg) milestone May 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.