Skip to content
This repository was archived by the owner on Mar 7, 2025. It is now read-only.

Exposes isSelected as a property#73

Merged
daniloercoli merged 3 commits intomasterfrom
try/add-is-selected-grab-focus
Nov 15, 2018
Merged

Exposes isSelected as a property#73
daniloercoli merged 3 commits intomasterfrom
try/add-is-selected-grab-focus

Conversation

@mzorz
Copy link
Copy Markdown
Contributor

@mzorz mzorz commented Nov 14, 2018

This PR exposes isSelected as a property. Each time the property is updated with the value true, we request the focus to be grabbed by the underlying AztecText view in Android.

This comes in handy for programmatically setting the focus / blinking cursor on a given AztecText view: by setting the isSelected property to true, an call to requestFocus() will be issued.

@daniloercoli
Copy link
Copy Markdown
Contributor

daniloercoli commented Nov 14, 2018

This looks fine and works as expected. Would you mind to check the default implementation of the same functionality in the bundled EditText component in ReactNative framework called ReactEditText ?

It seems that ReactTextInputManager has a command FOCUS_TEXT_INPUT, instead of a props. Would be good to use the same method. What do you think? There is also the other command BLUR_TEXT_INPUT to clearFocus.

  @Override
  public void clearFocus() {
    setFocusableInTouchMode(false);
    super.clearFocus();
    hideSoftKeyboard();
  }

  @Override
  public boolean requestFocus(int direction, Rect previouslyFocusedRect) {
    // Always return true if we are already focused. This is used by android in certain places,
    // such as text selection.
    if (isFocused()) {
      return true;
    }
    if (!mIsJSSettingFocus) {
      return false;
    }
    setFocusableInTouchMode(true);
    boolean focused = super.requestFocus(direction, previouslyFocusedRect);
    showSoftKeyboard();
    return focused;
  }

@mzorz
Copy link
Copy Markdown
Contributor Author

mzorz commented Nov 14, 2018

Thanks for pointing that out @daniloercoli ! I'm definitely going to play with that in subsequent PRs for sure 👍

Let me explain - I've been playing quite a bit with the onBlur, onFocus approach and others until I started removing stuff and deciding to only keep as minimum as needed. Certainly that component (ReactTextInput) handles lots of situations, but I thought it was a good idea to go step by step. Idea was to only have small changes through different PRs tackling problems one by one so it's easier first to make sense of each problem, and also to make it easier to review (not that I know exactly what I'm tackling next, but the idea still is to handle this in separate PRs). This specific PR here tries to tackle issue wordpress-mobile/gutenberg-mobile#113 for the Gutenberg RichText primitive (I just updated the description for wordpress-mobile/gutenberg-mobile#237 as well), while another one will handle it in PlainText based blocks.

I chose to use props isSelected instead of methods because of its simplicity, and we can keep the same meaning at all levels (block, Gutenberg primitive component, React Native wrapper, and Aztec).
I feel instead adding a method here, and add needed logic in upper levels (Gutenberg) to translate isSelected to a method call is not something we need right now.

That said, I'll play with that suggested implementation on other PRs to see if if helps fix other of the problems we may have (for example, wordpress-mobile/gutenberg-mobile#135).

@daniloercoli
Copy link
Copy Markdown
Contributor

I agree with you to go step by step. I also think it's much easier that we initially though. Gutenberg and GB-mobile will still use the isSelected property, instead, the RichText component in GB should translate it to a method call to the underlying AztecView.

Anyway i'm going to mark this as good and merge it.
cc @marecar3 since he may need the getFocus/dismissFocus feature in his branch.

@daniloercoli daniloercoli merged commit 7d27cdb into master Nov 15, 2018
@daniloercoli daniloercoli deleted the try/add-is-selected-grab-focus branch November 15, 2018 10:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants