Make PlainInput controllable #13795

buoyad · 2018-09-17T22:16:28Z

I took a detour trying to use HOCTimers with PlainInput, but that involved adding ref forwarding to HOCTimers, which it looks like redux isn't ready for yet (reduxjs/react-redux#1000). On the bright side it led me to improve the typing on PlainInput and things that 'implement' it.

This:

Adds a prop value to PlainInput that passes through to the native input component
Adds a function castProps to be used by components that use PropsWithInput so they can pass props through without flow complaining about extra props (a-la castPlatformStyles)
Implements transformText on the native component, setting the text and then delaying setting the selection to work around bad behavior on android
- transformText will throw an error if value is provided as a prop. In that case, the input is considered controlled. PlainInput exposes setSelection that can be used to set a selection that's compatible with the value prop. Conversely, if the input is uncontrolled setSelection will throw an error, as transformText should be used instead.
Adds a story Common/Controlled input playground with some examples and controls to test input behavior
Hooks up the amount value in the store to the send form field so requests from chat auto populate visually correctly
Forwards a ref from NewInput to PlainInput. (Flow doesn't know about React.forwardRef (see Missing React.forwardRef definition facebook/flow#6103) so there's a $FlowIssue there)

r? @keybase/react-hackers cc @akalin-keybase @cjb

…d update storyshots

zanderz

LGTM, I tried the storybook, but maybe get another thumb as well from someone who understands the significance better

akalin-keybase · 2018-09-19T00:34:22Z

shared/common-adapters/plain-input.js.flow

+  /**
+   *  This can only be used when the input is controlled. Use `transformText` if
+   *  you want to do this on an uncontrolled input. Make sure the Selection is
+   *  valid against the `value` prop. Avoid changing `value` and calling this at


Hmm...so now I'm confused by this API. We almost always want to change both the text and the selection at the same time...AIUI the existing use cases are inserting text (like emoji, mentions, etc.) at the cursor. So that's why I suggested having a selection prop along with the value. Did that not work?

I ran into issues early on with the parent actively managing the selection and value at the same time. AFAICT on desktop there's no way to listen to events that change the cursor position (without selecting anything). Without the parent knowing the selection beforehand, I think the semantics of the prop would be weird, and even then I'm not sure if there's a way to insert text at the cursor the way we want. I'm beginning to think having a transformText that works with controlled inputs would be a good idea, though I'm not sure how to handle that along with updating props.value...

Ah, yes, I remember running into that when I tried. @chrisnojima did mention there was some way to listen to all DOM events and then filter out the selection ones on a given component, but I didn't know enough to pursue that.

...and then, yeah, you can try implementing transformText on a controlled component, but then things get murky w.r.t. the source of truth...

@akalin-keybase OK, rather than going down the murky source of truth route, I added a getSelection method. I added an example of a controlled input setting both at the same time (getSelection -> change props.value -> setSelection). Let me know what you think

akalin-keybase · 2018-09-19T00:36:48Z

shared/common-adapters/plain-input.stories.js

@@ -46,6 +130,9 @@ const load = () => {
    .add('With placeholder color', () => (
      <PlainInput {...commonProps} placeholder="I'm blue!" placeholderColor="blue" />
    ))
+
+  // Sandbox for testing controlled input bugginess
+  storiesOf('Common', module).add('Controlled input playground', () => <ControlledInputPlayground />)


Cool! The main thing I wanted to test, though, was something that would insert/replace text. See the Input story 'Empty (uncontrolled)' and 'Empty (multiline) (uncontrolled)' for what I mean. Can you add a story like that? (Then it'll probably make it clear what I mean about being confused by the API...)

Added a replica of Empty (uncontrolled) (but controlled)

buoyad · 2018-09-20T15:52:30Z

CI will fail for now on prettier not liking this type arg

[error] shared/common-adapters/plain-input.stories.js: SyntaxError: Unexpected token (28:47)
[error]   27 |   // prettier-ignore
[error] > 28 |   _input = React.createRef<typeof PlainInput>()
[error]      |                                               ^

akalin-keybase

few more comments, and still gonna play around on mobile, but I think this approach would work!

akalin-keybase · 2018-09-21T19:03:05Z

shared/common-adapters/plain-input.stories.js

+      if (selection) {
+        this.setState(
+          s => {
+            const value = s.value.substring(0, selection.start) + t + s.value.substring(selection.start)


mind changing the last selection.start to selection.end? (to match the behavior of the uncontrolled version)

akalin-keybase · 2018-09-21T19:07:11Z

shared/common-adapters/plain-input.stories.js

+            const value = s.value.substring(0, selection.start) + t + s.value.substring(selection.start)
+            return {value}
+          },
+          () => {


ah, interesting. I was gonna ask how this would work with redux, but it looks like most (all?) of our inputs end up setting the state in some higher component anyway, so this would work!

Yeah I think having the value come directly from redux risks a bad ux, since there could be some latency involved and the keypress should be very responsive. So fortunately the ux and API interests align in this case

akalin-keybase · 2018-09-21T19:07:56Z

shared/common-adapters/plain-input.stories.js

+      </Box2>
+    )
+  }
+}


mind also adding a multiline version of this playground? sadly, I had found that some bugs only show up with multiline inputs...

akalin-keybase · 2018-09-21T20:31:17Z

shared/common-adapters/plain-input.stories.js

@@ -46,6 +180,9 @@ const load = () => {
    .add('With placeholder color', () => (
      <PlainInput {...commonProps} placeholder="I'm blue!" placeholderColor="blue" />
    ))
+
+  // Sandbox for testing controlled input bugginess
+  storiesOf('Common', module).add('Controlled input playground', () => <ControlledInputPlayground />)


Also perhaps this belongs under Common/Plain input in the storybook tree?

buoyad · 2018-09-21T20:55:37Z

Okay @akalin-keybase, I pushed the changes. From my testing on mobile I noticed that this change doesn't seem to have an effect on the ios simulator (it always inserts 'foo' at the end). I think the onSelectionChange events don't fire as reliably on the simulator.. I'll test it on device too.

MarcoPolo · 2018-09-25T17:16:47Z

shared/common-adapters/plain-input.desktop.js

@@ -73,6 +75,13 @@ class PlainInput extends React.PureComponent<InternalProps> {
  }

  transformText = (fn: TextInfo => TextInfo, reflectChange?: boolean) => {
+    const controlled = !!this.props.value


shouldn't you check if it's null or undefined instead? since empty string can be a valid value.

yup, thanks!

akalin-keybase

lgtm! tested on android

buoyad force-pushed the danny/DESKTOP-7572-new-controllable-inputs branch from a6b4b16 to bf6b917 Compare September 17, 2018 22:16

buoyad added 10 commits September 18, 2018 10:11

WIP

13b6192

better typing for plaininput and type casting function

4004de4

finish transform text and changes from testing

fa109e5

hook up buildingPayment.amount to send for asset input, add story, an…

7e6e920

…d update storyshots

change api a bit

b0a3344

add errors for illegal method use

93471da

forward ref from NewInput to PlainInput

f5ce402

add link to flow issue with forwardRef

3f33463

remove castProps and use destructure in NewInput instead

bb6a720

storyshots

309eaa3

buoyad force-pushed the danny/DESKTOP-7572-new-controllable-inputs branch from bf6b917 to 309eaa3 Compare September 18, 2018 14:16

buoyad added 2 commits September 18, 2018 10:29

remove outdated comment

820d9ca

fix storyshots from jest cache

368bc23

zanderz reviewed Sep 18, 2018

View reviewed changes

akalin-keybase reviewed Sep 19, 2018

View reviewed changes

buoyad added 2 commits September 20, 2018 10:38

Merge branch 'master' into danny/DESKTOP-7572-new-controllable-inputs

d5862e4

add getSelection and example of changing text and selection at same time

537429f

akalin-keybase reviewed Sep 21, 2018

View reviewed changes

buoyad added 2 commits September 21, 2018 16:55

review feedback

05d5357

native getProps cleanup

e80be0e

MarcoPolo reviewed Sep 25, 2018

View reviewed changes

MarcoPolo approved these changes Sep 25, 2018

View reviewed changes

better check for controlled

bbc8dc8

akalin-keybase approved these changes Sep 25, 2018

View reviewed changes

buoyad merged commit b2be704 into master Sep 26, 2018

buoyad deleted the danny/DESKTOP-7572-new-controllable-inputs branch September 26, 2018 14:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make PlainInput controllable #13795

Make PlainInput controllable #13795

buoyad commented Sep 17, 2018 •

edited

zanderz left a comment

akalin-keybase Sep 19, 2018

buoyad Sep 19, 2018

akalin-keybase Sep 19, 2018

buoyad Sep 20, 2018

akalin-keybase Sep 19, 2018

buoyad Sep 20, 2018 •

edited

buoyad commented Sep 20, 2018

akalin-keybase left a comment

akalin-keybase Sep 21, 2018

akalin-keybase Sep 21, 2018

buoyad Sep 21, 2018

akalin-keybase Sep 21, 2018

akalin-keybase Sep 21, 2018

buoyad commented Sep 21, 2018 •

edited by akalin-keybase

MarcoPolo Sep 25, 2018

buoyad Sep 25, 2018

akalin-keybase left a comment

Make PlainInput controllable #13795

Make PlainInput controllable #13795

Conversation

buoyad commented Sep 17, 2018 • edited

zanderz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buoyad Sep 20, 2018 • edited

Choose a reason for hiding this comment

buoyad commented Sep 20, 2018

akalin-keybase left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buoyad commented Sep 21, 2018 • edited by akalin-keybase

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akalin-keybase left a comment

Choose a reason for hiding this comment

buoyad commented Sep 17, 2018 •

edited

buoyad Sep 20, 2018 •

edited

buoyad commented Sep 21, 2018 •

edited by akalin-keybase