WIP: Autocomplete component by danseethaler · Pull Request #190 · patternfly/patternfly-react

danseethaler · 2018-01-26T20:51:31Z

What:
Adding a proposed auto-complete component. This component will show a dropdown of items while a user is typing. The items in the dropdown list are managed outside of the auto-complete component. The component implements the visual rendering of the items as well as keyboard support (tab, up, down, enter) and a11y accessibility. This component supports disabled items, headers, and dividers.

This is still a WIP and additional discussion is had in #189.

Link to Storybook:
https://rawgit.com/danseethaler/patternfly-react/autocomplete-component-storybook/index.html?selectedKind=AutoComplete&selectedStory=AutoComplete&full=0&addons=1&stories=1&panelRight=0&addonPanel=storybooks%2Fstorybook-addon-knobs

Additional issues:
Fixes #189

priley86 · 2018-01-26T23:01:00Z

+
+    return (
+      <Downshift
+        onStateChange={({ inputValue, ...rest }) => {


sooo nice ;)... render props are so powerful!

priley86 · 2018-01-26T23:03:29Z

+            <InputGroup>
+              <AutoCompleteInput
+                onKeyPress={e => {
+                  const TAB_KEY = 9;


maybe we can go ahead and move these to common/helpers.js ? export const KEY_CODES = { TAB_KEY: 9, ... }

I wish there was a way to destructure the properties but I like having them in the common folder.

sharvit

Nice @danseethaler 👍

While reviewing, I was surprised to see the show info button was disappeared from your storybook 😕

sharvit · 2018-01-29T16:01:21Z


 export const noop = Function.prototype;
+
+export const KEY_CODES = { TAB_KEY: 9, ENTER_KEY: 13 };


Is it really common?
If not, it should live under AutoComplete/constants.js

tbh, I'm not sure but let agree the first one outside of the autocomplete scope that wants to use it, will just move it into common/helpers?

Check out how the Alert is using inner helpers and constants:
https://github.com/patternfly/patternfly-react/tree/master/src/components/Alert

Great point. Thoughts @priley86?

keyboard key codes will be used very often when implementing a11y from a JS standpoint. it's mostly about handling enter press/escape press/tab press from the JS side...so for that reason, i would propose keeping it common to all.

sharvit · 2018-01-29T16:03:58Z

+import AutoCompleteItems from './AutoCompleteItems';
+import { KEY_CODES } from '../../common/helpers';
+
+export const getActiveItems = items =>


It should move into AutoComplete/helpers.js, see how it works in the Alert component:
https://github.com/patternfly/patternfly-react/tree/master/src/components/Alert

sharvit · 2018-01-29T16:20:21Z

+
+    return (
+      <Downshift
+        onStateChange={({ inputValue, ...rest }) => {


Let's try to keep this method lean.
Can you move this method into this.handleStateChange?

I like abstracting like that when possible. The extra challenge here is needed the onInputUpdate function in the scope. We could forward it through by currying or something but I think it's easier to see inline.

It's a prop, it should always be available under, this.props

sharvit · 2018-01-29T16:26:39Z

+        selectedItem={this.state.inputValue}
+        {...rest}
+      >
+        {({


It just looks really weird to pass a method as children in react JSX.
Downshift API support a render prop that can be used instead, I think it would feel better.

Yeah I'm totally open on this concept. It's just a preference since Downshift supports both. I'll change it and we'll take a look!

Having a render prop sounds interesting, as it would allow us to customize the options (e.g. think you would like to add an avatar in the user dropdown)

@ohadlevy yeah that's exactly the idea here. This implementation is pushing for a standardized auto-complete using the MenuItem component with just text but Downshift can be used to compose any kind of dropdown interface you want.

yep... render prop vs children as a function is just semantics i believe... either is fine w/ me 👍 ... i see people posting polls about this lol

sharvit · 2018-01-29T16:27:41Z

+            {labelText && <label {...getLabelProps()}>{labelText}</label>}
+            <InputGroup>
+              <AutoCompleteInput
+                onKeyPress={e => {


Let's try to keep this method lean.
Can you move this method into this.handleKeyPress?

Same thing here with all the closure scope we need. It would be nice to isolate but I don't see how without forward a bunch of variables which I'm not a fan of.

yea... that is one thing that appears different about render props... the scope is different. I was noticing the same a bit back in the Wizard and it was hitting my brain. I think you can bind it if you want, but it's not as clean.

Yeah I'm not a fan of binding in this context. I think it's easier to see what's happening inline even though it's long.

sharvit · 2018-01-29T16:41:16Z

+}
+
+AutoComplete.propTypes = {
+  items: PropTypes.arrayOf(PropTypes.object).isRequired,


Can you also add a description comment?
Checkout the Alert propTypes:
https://github.com/patternfly/patternfly-react/blob/master/src/components/Alert/Alert.js#L38

And how they compiled into the storybook:
https://rawgit.com/patternfly/patternfly-react/gh-pages/index.html?selectedKind=Alert&selectedStory=Alert%20types&full=0&addons=1&stories=1&panelRight=0&addonPanel=storybooks%2Fstorybook-addon-knobs
Click "show info" and scroll down to the prop types description.

Wow that's cool! Thanks for sharing 👍 Added them in now.

sharvit · 2018-01-29T16:41:49Z

@@ -0,0 +1,54 @@
+/* eslint-disable no-alert */


Forget it here?

Yeah guess this was an issue the foreman lint config but not a problem here. Good catch!

sharvit · 2018-01-29T16:42:21Z

+        </Col>
+      </Row>
+    </div>
+  ))


Really need this decorator?

The main purpose of this is to show how the dropdown will stick to the input field regardless of where it is on the screen.

I still don't understand why the general design won't address this need.

sharvit · 2018-01-29T16:44:25Z

+    this.ref && this.ref.removeEventListener('keydown', this.handeKeyPress);
+  }
+
+  handeKeyPress = e => {


handle.
you forgot the l

Your english is better than mine 👋

sharvit · 2018-01-29T16:57:44Z

+
+      return (
+        <MenuItem
+          {...getItemProps({


I think it would be more readable as a separate var

const itemProps = getItemProps({ ... }); <MenuItem {...itemProps}>{text}</MenuItem>;

sharvit

Looks much better,
I think it is very important to show a storybook with documentation.
Right now, if I am getting into the storybook, I have no information about how to use it in my project.

sharvit · 2018-01-30T08:35:32Z

+
+    return (
+      <Downshift
+        onStateChange={({ inputValue, ...rest }) => {


It's a prop, it should always be available under, this.props

sharvit · 2018-01-30T08:38:38Z

+        </Col>
+      </Row>
+    </div>
+  ))


I still don't understand why the general design won't address this need.

danseethaler · 2018-01-31T21:38:25Z

Thanks for the tips on the storybook info @sharvit. I haven't used that before so I patterned my info on the Alert component. Let me know what you think.

Also open to other thoughts/reviews! There is a new discussion in pf-design linked above that's worth checking out.

sharvit

Awesome, Thanks @danseethaler

One last minor issue with the AutoComplete class and I am really happy with the result here 👍

sharvit · 2018-02-01T10:53:15Z

+              {labelText && <label {...getLabelProps()}>{labelText}</label>}
+              <InputGroup>
+                <AutoCompleteInput
+                  onKeyPress={e => {


Can you move this function to be apart of the class like you did with handleStateChange?

Unfortunately we don't have the render prop props (say that three times fast :) in a class function so we'd have to do some pass-throughs which I'm not a fan of.

Cool, I understand your point now, it will actually be really messy...

sharvit · 2018-02-01T11:07:19Z

@danseethaler Can you also redeploy the storybook?
I had to run it locally in order to see it.

sharvit · 2018-02-01T12:03:16Z

+  withInfo({
+    source: false,
+    propTables: [AutoComplete],
+    propTablesExclude: [MockAutoComplete],


@priley86 This approach can fix some problems we are facing with the storybook?

@danseethaler 👍

@gilad215 /cc

👍 yes - that is the suggestion right now... nice job.

sharvit

Thanks @danseethaler, Feels really good 👍

danseethaler · 2018-02-01T14:50:38Z

+  selectedItem
+}) => (
+  <Dropdown.Menu
+    style={{


@priley86 while we're waiting for a pf-design for this component - is there a particular way we want to handle styling so it's not inline? I don't see any .scss files except at the top level to import pf core styles.

@danseethaler i've just been adding them here in the sass directory while demonstrating them in my PRs (then we can review it here first and consider adding it to PF Core after as a subsequent PR). Make sense? I believe @jgiardino is further detailing how to do this in #195

@priley86 ah perfect. I'll update accordingly.

sharvit · 2018-02-08T09:57:23Z

@danseethaler is it still WIP?
Feels to me like it's ready to review and merge...

danseethaler · 2018-02-08T13:22:30Z

@sharvit yeah I spoke with @priley86 and it sounds like the PF team would like the design confirmed in core before this component is merged. Hopefully that won't take too long but it may be another week or two.

dabeng · 2018-02-26T08:12:20Z

Hi @danseethaler . Awesome component 👍 . I used jquery version of autocomplete. I think Bootstrap-3-Typeahead and typeahead.js will give you inspirations if you want to add interesting features.

ohadlevy · 2018-03-18T09:26:15Z

@LaViro any chance you can take over this component? sadly I don't that @danseethaler will complete it in the near future :) thanks!

Ron-Lavi · 2018-03-18T20:33:23Z

@ohadlevy, sure.. I will do my best to (auto)complete this :)

… test. re patternfly#190

dabeng · 2018-06-04T04:33:21Z

It will be better to highlight key words with bold font.

affects: patternfly-react ISSUES CLOSED: patternfly#190 patternfly#281

priley86 reviewed Jan 26, 2018

View reviewed changes

sharvit suggested changes Jan 29, 2018

View reviewed changes

mcarrano mentioned this pull request Jan 29, 2018

Add Type-Ahead Input Pattern patternfly/patternfly-design#546

Closed

sharvit suggested changes Jan 30, 2018

View reviewed changes

danseethaler force-pushed the autocomplete-component branch from 14487e1 to f399287 Compare January 31, 2018 21:46

sharvit suggested changes Feb 1, 2018

View reviewed changes

sharvit reviewed Feb 1, 2018

View reviewed changes

sharvit previously approved these changes Feb 1, 2018

View reviewed changes

danseethaler commented Feb 1, 2018

View reviewed changes

danseethaler dismissed sharvit’s stale review via d554026 February 1, 2018 21:18

danseethaler force-pushed the autocomplete-component branch from d554026 to 40a366b Compare February 1, 2018 21:58

danseethaler mentioned this pull request Feb 1, 2018

Fixes #21953 Adding enable/disable and search bar Katello/katello#7180

Closed

1 task done

danseethaler added 10 commits February 5, 2018 10:19

feat(AutoComplete): Add AutoComplete component to lib

daea6f2

Add support for headers, dividers, and disabled items

49f0eeb

Add support for tab and enter keys

2e42fa0

Isolate Items to seperate file

9470522

Extract key_codes

330d5a1

Adding tests

88bba81

Recommended updates

2074363

Adding storybook info

3118d86

Test patch

6309069

Abstracting styles

d077833

danseethaler force-pushed the autocomplete-component branch from 40a366b to d077833 Compare February 5, 2018 15:19

priley86 mentioned this pull request Feb 12, 2018

Component - Select #54

Closed

sharvit mentioned this pull request Feb 13, 2018

feat(infoTip): Add Infotip component #199

Merged

waldenraines mentioned this pull request Mar 19, 2018

Fixes #21287 - add resource switcher theforeman/foreman#5188

Closed

Ron-Lavi pushed a commit to Ron-Lavi/patternfly-react that referenced this pull request Mar 20, 2018

feat(AutoComplete): Add the AutoComplete component with storybook and…

5d19906

… test. re patternfly#190

Ron-Lavi mentioned this pull request Mar 20, 2018

feat(AutoComplete): Add AutoComplete component with storybook and test #281

Closed

Ron-Lavi pushed a commit to Ron-Lavi/patternfly-react that referenced this pull request Mar 20, 2018

feat(AutoComplete): Add the AutoComplete component with storybook and…

802ea28

… test. re patternfly#190

Ron-Lavi pushed a commit to Ron-Lavi/patternfly-react that referenced this pull request Mar 21, 2018

feat(AutoComplete): Add the AutoComplete component with storybook and…

48e952a

… test. re patternfly#190

Ron-Lavi pushed a commit to Ron-Lavi/patternfly-react that referenced this pull request Mar 21, 2018

feat(AutoComplete): Add the AutoComplete component with storybook and…

7c88160

… test. re patternfly#190

This was referenced May 31, 2018

feat(Autocomplete): add Autocomplete component with storybook and tests. #373

Closed

feat(TypeAheadSelect): Add TypeAheadSelect component with storybook and tests #374

Merged

Ron-Lavi pushed a commit to Ron-Lavi/patternfly-react that referenced this pull request Jun 13, 2018

feat(TypeAheadSelect): add TypeAheadSelect component

7d626a0

affects: patternfly-react ISSUES CLOSED: patternfly#190 patternfly#281

jeff-phillips-18 closed this in 79fe82d Jun 13, 2018


		export const noop = Function.prototype;

		export const KEY_CODES = { TAB_KEY: 9, ENTER_KEY: 13 };

Conversation

danseethaler commented Jan 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

priley86 Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharvit left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharvit Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

priley86 Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharvit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danseethaler commented Jan 31, 2018

Uh oh!

priley86 Jan 26, 2018 •

edited

Loading

sharvit left a comment •

edited

Loading

sharvit Jan 29, 2018 •

edited

Loading

priley86 Jan 29, 2018 •

edited

Loading

dabeng commented Feb 26, 2018 •

edited

Loading