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

[Android] setSelection for Aztec wrapper #629

Merged
merged 4 commits into from
Feb 22, 2019

Conversation

hypest
Copy link
Contributor

@hypest hypest commented Feb 20, 2019

Fixes #602

This PR introduces a "setSelection" functionality to the Aztec wrapper component. It is currently wired up to the RichText component as a prop and updated every time the locally kept selectionStart and selectionEnd changes. See WordPress/gutenberg#13978.

With this setting of selection, the caret position is set to 0 for the newly created block from the block split.

This PR adds a MovementMethod that places the caret at the beginning of text when focusing the view. The Android default is to place is at the end, but in our case, placing it at the start is more appropriate.

To test:

  1. In a paragraph block that has some text, place the caret in the middle of the text
  2. Tap "Enter" to split the block
  3. Notice the caret being placed at the beginning of the new block

Currently, it just mirrors to Aztec what the internal state vars hold.
This also has the desirable effect of setting the selection to 0,0 on
block split.
@daniloercoli
Copy link
Contributor

I noticed that sometimes when tapping on another block, say you tap between two words, the caret is initially placed at the "point of the tap", but immediately after moved to the end of the text.
https://cloudup.com/ckcVwELlawk

To reproduce the issue

  1. Tap on a Block_A and start adding new content to it
  2. Tap in the middle of a different RichText block (the caret is properly placed)
  3. Tap back in the middle of the Block_A
  4. The caret is moved to the end of Block_A

(to replicate the issue you need to modify the content of the first block).

@hypest
Copy link
Contributor Author

hypest commented Feb 21, 2019

Confirmed. Will need to fix that.

@hypest
Copy link
Contributor Author

hypest commented Feb 22, 2019

I've revised the solution to take another route: Removed the setting of selection, and instead, added a MovementMethod that places the caret at the beginning of text when focusing the view. The Android default is to place is at the end, but in our case, placing it at the start is more appropriate.

Now, we might want to place the caret at the end in some cases, and we shall make the custom MovementMethod smarter then.

Ready for another pass @daniloercoli , thanks!

Btw, the Gutenberg-side PR is no longer needed and will close it when we finalize this PR.

@hypest hypest requested review from daniloercoli and removed request for daniloercoli February 22, 2019 00:35
@hypest hypest modified the milestones: Beta, v1.0 Feb 22, 2019
@hypest hypest requested a review from mzorz February 22, 2019 12:15
Copy link
Contributor

@mzorz mzorz left a comment

Choose a reason for hiding this comment

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

code LGTM, tried it quite a bit and it works as advertised, only thing I noticed is some lag when splitting the blocks but it doesn't seem to be a problem of this PR in particular.
I tried using large pieces of text and short words and the lag seems to be the same on both, so, doesn't seem related to content length.

:shipit:

@hypest hypest merged commit 040d957 into develop Feb 22, 2019
@hypest hypest deleted the issue/602-set-cursor-to-start-on-split branch February 22, 2019 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants