feat(Input): adding variation for clearable input #37

alinais · 2018-08-01T14:17:15Z

Input clear icon variation

Adding variation for clearing the input on click on Cancel icon

TODO

API Proposal

default:

typing text:

click on remove text:

levithomason

I've left some comments on cleaning up the code for the current direction.

I would also like to discuss changing the course a little bit. It would be ideal if the user could just add a clearable prop to the Input and get the behavior for free. In that case, it would achieved using the AutoControlledComponent for now. Feedback?

levithomason · 2018-08-02T04:40:17Z

docs/src/examples/components/Input/Variations/InputExampleIconChange.shorthand.tsx

+class InputExampleIconChangeShorthand extends React.Component<
+  {},
+  { icon: string; inputValue: string }
+> {


No typings in examples, please. The public facing code is not typescript but JS. We want to advertise the most minimal syntax possible to users in order to focus on our features. We also don't want to assume they are using typescript since this is a small percentage of the community.

levithomason · 2018-08-02T04:41:10Z

docs/src/examples/components/Input/Variations/InputExampleIconChange.shorthand.tsx

+      icon: 'search',
+      inputValue: '',
+    }
+  }


Can be simplified to:

state = { value: '' }

For simplicity, I've also renamed the inputValue key above to value to reduce more boilerplate below.

I removed icon as there is no need to persist this on state since it can be computed inline in the render function.

levithomason · 2018-08-02T04:44:32Z

docs/src/examples/components/Input/Variations/InputExampleIconChange.shorthand.tsx

+      icon: value ? 'close' : 'search',
+      inputValue: value,
+    })
+  }


Example refactor given the above comment:

handleChange = (e, { value }) => this.setState({ value })

levithomason · 2018-08-02T04:46:18Z

docs/src/examples/components/Input/Variations/InputExampleIconChange.shorthand.tsx

+        placeholder="Search..."
+        value={inputValue}
+        onChange={this.handleChange}
+        onIconClick={e => this.onIconClick(e, icon)}


Avoid creating functions in the render method. This is an anti-pattern since it causes the prop to receive a new value on every render. This means you cannot compare the previous and current value of this prop for optimizations.

That said, I'm not sure we should have this prop since the user can pass an onClick prop to the icon shorthand prop:

<Input icon={{ name: 'search', onClick: this.handleIconClick }} />

Thoughts?

Cannot think of a real example of the usage of the onIconClick handler right now. I believe that the callback can be added to the onClick property of an Icon and same thing should happen. I can remove the extra handler for now and add it further if needed.

levithomason · 2018-08-02T04:46:58Z

docs/src/examples/components/Input/Variations/InputExampleIconChange.shorthand.tsx

+  public onIconClick = (e, value) => {
+    if (value === 'close') {
+      this.setState({
+        icon: 'search',


It is safe to not have icon here since setting value to empty will result in a re-render and the correct icon will be derived from state.

I introduced the clearable prop that will take care of it

levithomason · 2018-08-02T04:47:48Z

docs/src/examples/components/Input/Variations/InputExampleIconChange.shorthand.tsx

+
+    return (
+      <Input
+        icon={icon}


Per the above, you can this: icon={value ? 'close' : 'search'}.

change the approach

levithomason · 2018-08-02T04:59:03Z

src/components/Input/Input.tsx

+          ...icon,
+          ...(icon.onClick && { tabIndex: '0' }),
+        }),
+    }


You can avoid the logic required to create these props by adding the tabIndex in the overrideProps function:

computeTabIndex = props => { if (!_.isNil(props.tabIndex)) return props.tabIndex if (props.onClick || props.onIconClick) return 0 } Icon.create(icon, { overrideProps: predefinedProps => ({ tabIndex: this.computeTabIndex(predefinedProps) }) })

The predefinedProps handed to the overrideProps function are the final computed props of the factory function. The factory has already taken into account all factors necessary to compute the final props.

sounds like a good idea, thanks

codecov · 2018-08-03T16:01:52Z

Codecov Report

Merging #37 into master will decrease coverage by 0.01%.
The diff coverage is 72.72%.

@@            Coverage Diff             @@
##           master      #37      +/-   ##
==========================================
- Coverage   88.39%   88.37%   -0.02%     
==========================================
  Files          45       45              
  Lines         741      757      +16     
  Branches       97      109      +12     
==========================================
+ Hits          655      669      +14     
- Misses         83       85       +2     
  Partials        3        3

Impacted Files	Coverage Δ
src/components/Input/Input.tsx	`78.68% <72.72%> (+3.13%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f5b35af...45fc37f. Read the comment docs.

alinais · 2018-08-06T11:24:29Z

@levithomason I addressed the requested changes. Please have a look.

layershifter · 2018-08-06T11:27:19Z

src/components/Input/Input.tsx

+      value: props.value || '',
+    }
+
+    this.handleChange = this.handleChange.bind(this)


Looks like useless line because handleChange is an arrow function.

https://medium.com/@machnicki/handle-events-in-react-with-arrow-functions-ede88184bbb

layershifter · 2018-08-06T11:27:54Z

src/components/Input/Input.tsx

+
+  computeTabIndex = props => {
+    if (!_.isNil(props.tabIndex)) return props.tabIndex
+    if (props.onClick) return 0


I think that desctruction is preferred in this case.

layershifter · 2018-08-06T11:29:14Z

src/components/Input/Input.tsx

+    const inputValue = _.get(e, 'target.value')
+    const { clearable } = this.props
+
+    _.invoke(this.props, 'onChange', e, { ...this.props, inputValue })


I think that inputValue should be just value, too

layershifter · 2018-08-06T11:30:00Z

docs/src/examples/components/Input/Variations/InputExampleIconClearable.shorthand.tsx

+  public render() {
+    return <Input icon="search" clearable placeholder="Search..." />
+  }
+}


This example can be a stateless component.

levithomason · 2018-08-09T17:11:59Z

docs/src/examples/components/Input/Variations/index.tsx

@@ -9,6 +9,11 @@ const Variations = () => (
      description="An input can have an icon."
      examplePath="components/Input/Variations/InputExampleIcon"
    />
+    <ComponentExample
+      title="Clearable icon"


"Clearable", titles should be the prop name when possible.

levithomason · 2018-08-09T17:15:12Z

src/components/Input/Input.tsx

+    this.state = {
+      value: props.value || '',
+    }
+  }


Looks like we have a default prop here. Also, no need for a constructor:

state = { value: this.props.value } static defaultProps = { value: '' }

This will ensure the default value is advertised.

levithomason · 2018-08-09T17:26:50Z

src/components/Input/Input.tsx

+
+    _.invoke(this.props, 'onChange', e, { ...this.props, value })
+
+    this.setState({ value })


If the user passes a value prop, their value will no longer be respected. Clear should only work if the user does not have a value prop set.

You can paste this in an example to try it:

import React from 'react' import { Input } from '@stardust-ui/react' const InputExampleIconClearableShorthand = () => ( <Input clearable icon="search" placeholder="Search..." value='keep me' /> ) export default InputExampleIconClearableShorthand

The value of props should always be reflected in the DOM. This is idiomatic React. Just as when using an input and passing a value prop, the input does not update when typing in it unless you capture that event and update the prop also. User props always win.

The AutoControlledComponent will handle this logic for you if you want to use that for now. Otherwise, you will need to implement it yourself until we get the state manager for inputs. I would not advise doing that as there are many edge cases (see AutoControlledComponent).

levithomason · 2018-08-09T17:50:51Z

docs/src/examples/components/Input/Variations/index.tsx

@@ -9,6 +9,11 @@ const Variations = () => (
      description="An input can have an icon."
      examplePath="components/Input/Variations/InputExampleIcon"
    />
+    <ComponentExample
+      title="Clearable icon"
+      description="An input can have a search icon that can change into clear button on typing."


This feature is only showing that an input can make its value clearable.

The use of the search icon is unrelated and should not be in the description. In fact, it may warrant a separate example showing that the clearable icon will override a normal icon when there is a value. The simplest example doesn't actually require an existing icon in the input, correct?

I'm trying to imagine the example where the input is clearable and doesn't have an icon. I might misunderstand the original ask...

For example, I would expect as a user that I could do this:

<Input clearable />

And it would result in an input that can clear its value. As a user, I do not have to also supply an icon='search' prop in order for the clearable feature work. I can supply one if I wish, and it will be replaced with an x when there is an input value, but it is not required for the clearable feature to work.

SUIR Loading Input

Here's a corresponding example from SUIR. An Input can be in the loading state. The input can be defined with or without an icon prop. Here is the same loading input, but shown with the icon defined:

<Input placeholder='With an icon' icon='search' loading /> <Input placeholder='Without an icon' loading />

Here I am toggling the loading prop:

The clearable prop would work similar. The use of the icon='search' is not required for the clearable icon to show up when there is a value.

levithomason

Let's clean up docs a bit. Also, the user's value prop is not being respected. See comments.

levithomason · 2018-08-16T02:34:15Z

src/components/Input/Input.tsx

+
+    _.invoke(this.props, 'onChange', e, { ...this.props, value })
+
+    !this.props.value && this.setState({ value })


AutoControlledComponent allows you to this.trySetState() which will automatically handle the proper checks against props of the same name as the state keys you are trying to set.

levithomason · 2018-08-16T02:35:24Z

src/components/Input/Input.tsx

+      this.setState({ value: '' })
+    } else if (clearable && this.props.value && this.props.value.length !== 0) {
+      this.setState({ value: this.props.value })
+    }


This can safely be replaced with:

if (clearable) this.trySetState({ value: '' })

This method handles these checks and more edge cases. Also, note that when using AutoControlledComponent you no longer access this.props for the value, but this.state only.

levithomason · 2018-08-16T04:14:20Z

src/components/Input/Input.tsx

+
+  getInitialAutoControlledState() {
+    return { value: '' }
+  }


The state = {} on line 96 seems to conflict with the getInitialAutoControlledState here on 98. The result of getInitialAutoControlledState is applied after the state member.

levithomason

When applying defaultValue, the value is not respected on first render. As an example, I had updated the examples to use the below code to make it more clear that the input can be cleared:

<Input clearable defaultValue="Clear me" placeholder="Search..." />

This does not render an input with a value of Clear me. The current PR seems to be overriding the AutoControlledComponent's handling of the default props.

…erty usage; keeping the value provided by the user for the input as the meaningful one

…iation

levithomason · 2018-08-20T21:54:54Z

Heads up, I merged master here to get latest CI speed up benefits.

levithomason · 2018-08-20T21:55:24Z

docs/src/examples/components/Input/Types/InputExample.shorthand.tsx

@@ -1,4 +1,4 @@
-import React from 'react'
+import * as React from 'react'


No TS in examples please.

levithomason · 2018-08-20T21:58:33Z

test/specs/components/Input/Input-test.ts

-
-describe('Input', () => {
-  isConformant(Input)
-})


💡 Tip

You can use git mv <old-path> <new-path> to move files. This will ensure git can track changes between filenames instead of appearing as a file delete and and file create. It improves readability and also makes rebase/merge more robust.

levithomason

Changes look good here. Left a couple comments for consideration but not blocking merge. Any further updates we can make it separate PRs.

…iation

alinais requested review from dvdzkwsk, kuzhelov, levithomason and mnajdova as code owners August 1, 2018 14:17

levithomason suggested changes Aug 2, 2018

View reviewed changes

alinais force-pushed the feature/input-component-clearable-icon-variation branch from da9d255 to 47dbe7c Compare August 3, 2018 10:37

layershifter reviewed Aug 6, 2018

View reviewed changes

alinais force-pushed the feature/input-component-clearable-icon-variation branch 3 times, most recently from 937af84 to 0e8584e Compare August 8, 2018 11:00

levithomason reviewed Aug 9, 2018

View reviewed changes

levithomason suggested changes Aug 9, 2018

View reviewed changes

alinais force-pushed the feature/input-component-clearable-icon-variation branch from 0e8584e to 92ad327 Compare August 15, 2018 15:01

levithomason reviewed Aug 16, 2018

View reviewed changes

levithomason suggested changes Aug 16, 2018

View reviewed changes

alinais added the 🚀 ready for review label Aug 16, 2018

alinais added 4 commits August 16, 2018 16:16

adding variation for clearable input

463a11c

adding clearable property to Input component

554e7a9

fixing merge issues; fixing the inputs without clearable prop

36ce1d5

making the Input component a normal UIComponent

60ef99b

alinais added 6 commits August 16, 2018 16:16

making the value prop working

8e7a736

cleaning the Input code; cleaning the example

440ecbc

fixing failing tests

9c0b991

updating changelog

653e3c2

updating the input variations with a simple example of clearable prop…

739d804

…erty usage; keeping the value provided by the user for the input as the meaningful one

use trySet State instead of setState

97e22b1

alinais force-pushed the feature/input-component-clearable-icon-variation branch from e14840d to 97e22b1 Compare August 16, 2018 14:16

Merge branch 'master' into feature/input-component-clearable-icon-var…

bcbef98

…iation

alinais requested a review from miroslavstastny as a code owner August 20, 2018 21:54

levithomason reviewed Aug 20, 2018

View reviewed changes

levithomason approved these changes Aug 20, 2018

View reviewed changes

levithomason added ready for merge and removed 🚀 ready for review labels Aug 20, 2018

Merge branch 'master' into feature/input-component-clearable-icon-var…

45fc37f

…iation

alinais closed this Aug 21, 2018

alinais reopened this Aug 21, 2018

alinais closed this Aug 21, 2018

alinais reopened this Aug 21, 2018

alinais merged commit 62ac7b5 into master Aug 21, 2018

levithomason deleted the feature/input-component-clearable-icon-variation branch October 11, 2018 21:23


		_.invoke(this.props, 'onChange', e, { ...this.props, value })

		this.setState({ value })


		_.invoke(this.props, 'onChange', e, { ...this.props, value })

		!this.props.value && this.setState({ value })

		@@ -1,4 +1,4 @@
		import React from 'react'
		import * as React from 'react'

feat(Input): adding variation for clearable input #37

feat(Input): adding variation for clearable input #37

Conversation

alinais commented Aug 1, 2018 • edited Loading

Input clear icon variation

TODO

API Proposal

levithomason 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

alinais Aug 2, 2018 • edited Loading

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

codecov bot commented Aug 3, 2018 • edited Loading

Codecov Report

alinais commented Aug 6, 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

levithomason Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levithomason Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SUIR Loading Input

levithomason left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levithomason Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levithomason left a comment • edited Loading

Choose a reason for hiding this comment

levithomason commented Aug 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levithomason left a comment

Choose a reason for hiding this comment

alinais commented Aug 1, 2018 •

edited

Loading

alinais Aug 2, 2018 •

edited

Loading

codecov bot commented Aug 3, 2018 •

edited

Loading

levithomason Aug 9, 2018 •

edited

Loading

levithomason Aug 9, 2018 •

edited

Loading

levithomason Aug 16, 2018 •

edited

Loading

levithomason left a comment •

edited

Loading