feat: implement helper and error text support in TextInput #243

kpsroka · 2018-02-16T15:41:23Z

Please take a look at the example app for a demo of how it looks like/works. For now, I didn't want to refactor this into a separate element -- we should do that once we know what it should look like in other components. One not implemented feature is to have an icon in the error text. The material docs use error-outline, but we might consider allowing any (or any material?) icon source.

(Resolves #175)

callstack-bot · 2018-02-16T15:42:53Z

Hey @kpsroka, thank you for your pull request 🤗. The documentation from this branch can be viewed here.

kpsroka · 2018-02-26T12:27:41Z

@satya164 Can you please take a look at this?

satya164

I still think it's better to just separate helper text into a separate component.

<TextInput error={true} text={this.state.text} onTextChange={text => this.setState({ text })} />
<HelperText error={true}>
  Please enter at least 3 characters
</HelperText>

This looks a little bit verbose, but we can add a Form component later which handles validation for various components and reduces boilerplate.

For now, I didn't want to refactor this into a separate element -- we should do that once we know what it should look like in other components

Keep in mind that once this is merged to master, we can't change it without making it a breaking change, and I'd like to avoid that at such a short interval after the release. If you want to check how other libs do it, here is an example: https://material-ui-next.com/api/form-helper-text/

satya164 · 2018-03-07T15:49:48Z

src/components/TextInput.js

+  /**
+   * Whether to style the TextInput with error style.
+   */
+  hasError: boolean,


I think instead of this, a validate prop might be better,

validate={text => text.length < 3 ? new Error('Please enter at least 3 characters') : null}

There are 2 advantages: first, it can be used with both controlled and uncontrolled inputs, currently, you need to maintain a state in your component to do validation. second, it reduces duplication since the logic of both checking whether there is an error and the error message are in the same place, and makes impossible states impossible, e.g. the user cannot pass hasError={true} without an error text.

I think this should wait for the FormInput helper.

satya164 · 2018-03-07T15:50:42Z

src/components/TextInput.js

+        : 0;
+
+    /* Move label to top if value is set */
+    const labelTranslateY = value
      ? -22


Let's extract this number to a constant to the top of the file

satya164 · 2018-03-07T15:50:54Z

src/components/TextInput.js

      ? -22
      : this.state.focused.interpolate({
          inputRange: [0, 1],
          outputRange: [0, -22],
        });
-    const fontSize = value
+
+    const labelFontSize = value
      ? 12


Extract to a constant

satya164 · 2018-03-07T15:51:39Z

src/components/TextInput.js

-            style={[styles.bottomLine, { backgroundColor: inactiveColor }]}
+            style={[
+              styles.bottomLine,
+              { backgroundColor: (hasError && errorColor) || inactiveColor },


hasError ? errorColor : inactiveColor looks better

satya164 · 2018-03-07T15:52:46Z

src/components/TextInput.js

+            style={[
+              styles.bottomLine,
+              styles.focusLine,
+              bottomLineStyle(bottomLineColor, this.state.focused),


Let's rename it to something like getBottomLineStyle

satya164 · 2018-03-07T15:55:24Z

src/components/TextInput.js

          />
        </View>
+        {(helperText || errorText) && (


This will return the helperText/errorText if they are empty string and {(helperText || errorText) && (...)} will evaluate to '', RN will throw an error saying that it's not possible to render raw string outside text. let's change this to a ternary

satya164 · 2018-03-07T15:55:55Z

src/styles/DarkTheme.js

@@ -22,6 +22,11 @@ const DarkTheme: Theme = {
      .alpha(0.38)
      .rgb()
      .string(),
+    helperText: color(white)


We don't put component specific styles in theme. Let's move these to the component

I removed helperText from here, however I think that errorText is generic enough and helpful enough (for custom theming) to warrant its inclusion here.

kpsroka · 2018-03-08T11:23:46Z

How would you like the extracted component to be used? Let's assume we have a HelperText component. We have the following options:

We let users use HelperText without restrictions, then they need to wrap it in some other View that aligns their components with HelperText. So, there's a lot of repetition on the client side, since there's a very common use case of having them under TextInput. Also, the users need to handle potential resizing of the HelperText, which can lead to error-prone code.
1. We can add a prop to TextInput (and other common cases) to use a custom component in the underline area, and manage potential issues with resizing as well. This is quite permissive, and can lead to users abusing this and ending up writing crazy issues.
We keep HelperText internal, and either:
1. spread the props to TextInput (and other form components), or
2. introduce a helperTextProps prop to pass on to HelperText (and disable HelperText altogether).

I'm in favor of the last option. This makes sure that users don't need to spend time on possibly error-prone code. On the other hand, it encapsulates the HelperText from other components, so that breaking changes in HelperText are restricted to users of that feature.

satya164 · 2018-03-08T13:29:01Z

but we can add a Form component later which handles validation for various components and reduces boilerplate

kpsroka · 2018-03-28T12:31:21Z

Hi @satya164 ,
As requested, I split most of the functionality into a separate component. The changes in TextInput are to support red coloring and wiggle animation of minimized label. I don't think introducing the 'verify' prop in TextInput is a good think -- the upcoming FormInput seems like a better place for such functionality.

PTAL.

kpsroka · 2018-04-05T13:21:36Z

@satya164 Please review this PR again.

satya164 · 2018-04-15T19:45:42Z

src/types.js

    text: string,
    disabled: string,
    placeholder: string,
+    errorText: string,


We don't usually put component specific styles in theme. Also it's not clear what's the difference between error and errorText here. Let's move this one to the component.

satya164 · 2018-04-15T19:46:53Z

src/components/HelperText.js

+
+type Props = {
+  /**
+   * @optional


You don't need @optional. React docgen will infer it from flow types.

satya164 · 2018-04-15T19:48:12Z

src/components/HelperText.js

+   * @optional
+   * Text color to use.
+   */
+  color?: string,


Maybe we don't need this props since people can already override the color with the theme prop.

Which theme color should be used here? The MD specs differentiate between the placeholder and helper text colors. Should we introduce a helper text color in the theme?

satya164 · 2018-04-15T19:50:45Z

src/components/HelperText.js

+
+  state: State;
+
+  componentWillReceiveProps(nextProps) {


componentWillReceiveProps is deprecated, so should use getDerivedStateFromProps and use react-lifecycle-compat to support older version.

Though in this case, since it does a side-effect, we should use componentDidUpdate instead.

satya164 · 2018-04-15T19:52:24Z

src/components/HelperText.js

+
+  componentWillReceiveProps(nextProps) {
+    if (nextProps.hasError !== this.props.hasError) {
+      (nextProps.hasError ? this._animateFocus : this._animateBlur)(


Will be clearer to do:

if (nextProps.hasError) { this._animateFocus(); } else { this._animateBlur(); }

satya164 · 2018-04-15T22:21:39Z

src/components/TextInput.js

@@ -13,6 +13,11 @@ import type { Theme } from '../types';

 const AnimatedText = Animated.createAnimatedComponent(Text);

+const minimizedLabelYOffset = -22;


We usually use UPPER_SNAKE_CASE for constants

satya164 · 2018-04-15T22:23:51Z

src/components/HelperText.js

+
+  getHelperTextColor(dark: boolean) {
+    return dark
+      ? colorModule(white)


Let's do:

color(theme.colors.text) .alpha(dark ? 0.7 : 0.54) .rgb() .string();

satya164 · 2018-04-15T22:25:43Z

src/components/HelperText.js

+    return (
+      text && (
+        <Animated.View style={containerStyle}>
+          {text && <Text style={[styles.helperText, { color }]}>{text}</Text>}


Redundant check

satya164 · 2018-04-15T22:28:19Z

src/components/TextInput.js

+  /**
+   * Whether to style the TextInput with error style.
+   */
+  hasError: boolean,


Maybe just error: boolean since we usually skip prefixes everywhere else, like disabled, multiline

satya164 · 2018-04-15T22:29:25Z

src/components/HelperText.js

+const styles = StyleSheet.create({
+  helperText: {
+    fontSize: 12,
+    marginTop: 4,


Maybe this should be marginVertical: 4?

kpsroka · 2018-04-20T14:11:38Z

I applied the requested changes, let's take a look at it together on Tuesday and merge.

kpsroka · 2018-04-26T14:25:00Z

@satya164 I applied the changes that we discussed today, but I kept the hasError prop name.

satya164

The screenshots are too small in retina. The size should be same as the ones in TextInput docs. Also, need some simple snapshot tests.

If you have time to fix these, let's do it, or I'll do it when I'm back.

…#243)

kpsroka requested a review from satya164 February 16, 2018 15:42

kpsroka changed the title ~~feat: implement helper and error text support in TextField~~ feat: implement helper and error text support in TextInput Feb 19, 2018

kpsroka assigned satya164 Feb 20, 2018

satya164 requested changes Mar 7, 2018

View reviewed changes

satya164 force-pushed the master branch from 42dcef9 to 1cb7958 Compare March 19, 2018 10:35

ferrannp added the wip label Mar 21, 2018

ferrannp removed the wip label Apr 4, 2018

satya164 force-pushed the master branch from 7839080 to fa352f3 Compare April 11, 2018 13:33

satya164 requested changes Apr 15, 2018

View reviewed changes

satya164 mentioned this pull request Apr 16, 2018

WIP: Feat/more flexible text input #316

Closed

ferrannp added wip core team labels Apr 18, 2018

kpsroka removed the wip label Apr 20, 2018

satya164 force-pushed the feat/helper-text branch from 7cb9ffc to aa3bee7 Compare May 1, 2018 09:03

satya164 requested changes May 1, 2018

View reviewed changes

satya164 force-pushed the feat/helper-text branch 2 times, most recently from 9e27da8 to 2fda6f8 Compare May 1, 2018 09:37

feat: adding basic helperText support to TextInput

ea7109e

satya164 force-pushed the feat/helper-text branch from 2fda6f8 to ea7109e Compare May 5, 2018 16:03

satya164 approved these changes May 5, 2018

View reviewed changes

satya164 merged commit 7c054c0 into master May 5, 2018

satya164 deleted the feat/helper-text branch May 5, 2018 16:06

eriveltonelias pushed a commit to eriveltonelias/react-native-paper that referenced this pull request Sep 21, 2018

feat: implement helper and error text support in TextInput (callstack…

e00cc9b

…#243)

		@@ -13,6 +13,11 @@ import type { Theme } from '../types';

		const AnimatedText = Animated.createAnimatedComponent(Text);

		const minimizedLabelYOffset = -22;

feat: implement helper and error text support in TextInput #243

feat: implement helper and error text support in TextInput #243

Conversation

kpsroka commented Feb 16, 2018 • edited Loading

callstack-bot commented Feb 16, 2018 • edited Loading

kpsroka commented Feb 26, 2018

satya164 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpsroka Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

kpsroka commented Mar 8, 2018

satya164 commented Mar 8, 2018

kpsroka commented Mar 28, 2018

kpsroka commented Apr 5, 2018

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

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

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

kpsroka commented Apr 20, 2018

kpsroka commented Apr 26, 2018

satya164 left a comment • edited Loading

Choose a reason for hiding this comment

kpsroka commented Feb 16, 2018 •

edited

Loading

callstack-bot commented Feb 16, 2018 •

edited

Loading

kpsroka Mar 8, 2018 •

edited

Loading

satya164 left a comment •

edited

Loading