Skip to content

Conversation

@austingreendev
Copy link
Contributor

Closes #51

Description

This PR introduces a simplified, high-abstraction element for common Autocomplete patterns. The basic API is:

<Autocomplete
  label="Basic Autocomplete"
  placeholder="Filter values"
  selectedValue={state.selectedValue}
  onChange={selectedValue => setState({ selectedValue })}
  options={[
    {
      value: 'option-1',
      label: 'Option 1'
    },
    {
      value: 'option-2',
      label: 'Option 2'
    },
    {
      value: 'option-3',
      label: 'Option 3'
    }
  ]}
/>;

This component also abstracts out much of the logic around filtering and rendering, but allows deeper customization through the renderOption and renderDropdown render-props as well as the ability to override the default text comparison with a custom comparator.

Checklist

  • 👌 design updates are Garden Designer approved (add the
    designer as a reviewer)
  • 💅 view component styling is based on a Garden CSS
    component
  • 🌐 Styleguidist demo is up-to-date (yarn start)
  • ⬅️ renders as expected with reversed (RTL) direction
  • 💂‍♂️ includes new unit and snapshot tests
  • 📝 tested in Chrome, Firefox, Safari, Edge, and IE11

},
collectCoverageFrom: [
'<rootDir>/packages/*!(.template)/src/**/*.{js,jsx}',
'<rootDir>/packages/*/src/**/*.{js,jsx}',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My addition of the index.spec.js tests broke our code coverage 😢 This re-enables that feature

@ryanseddon
Copy link
Contributor

I think we should copy the same way we've done radios/checkboxes, rather than pass a prop for label, hint & message we have the children inside the <Autocomplete /> component.

e.g.

<Autocomplete
  placeholder="Filter values"
  selectedValue={state.selectedValue}
  onChange={selectedValue => setState({ selectedValue })}
  options={[...]}
>
  <Label>Basic Autocomplete</Label>
  <Hint>Autocomplete hint</Hint>
  <Message validation="error">Error</Message>
</Autocomplete>

This way they can still pass validation to <Message> and it's up to the consumer to explicitly include them.

@austingreendev
Copy link
Contributor Author

I think we should copy the same way we've done radios/checkboxes, rather than pass a prop for label, hint & message we have the children inside the <Autocomplete /> component.

I originally tried this, but had trouble getting everything to play nice together since we are stitching the FieldContainer and AutocompleteContainers together.

Also, with the example above, would the Autocomplete container have to conditionally render the "input" depending on the type of elements provided? Not sure creating separate AutocompleteField and Autocomplete components would help much since this is already such an opinionated component.

Copy link
Member

@jzempel jzempel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update examples/autocomplete.md with this new element vs. the existing container usage.

@ryanseddon
Copy link
Contributor

Also, with the example above, would the Autocomplete container have to conditionally render the "input" depending on the type of elements provided? Not sure creating separate AutocompleteField and Autocomplete components would help much since this is already such an opinionated component.

Fair enough maybe a good compromise would be to allow passing through a string or node to label, hint & message props.

@jzempel
Copy link
Member

jzempel commented Dec 15, 2018

@allisonacs @Austin94 we need to continue to think about the normative gesture here. Consider the tab action through native form elements. On <input> fields, we get automatic range selection of any existing value (allowing for efficient replacement) and on <textarea> the cursor is located at the end of the existing value.

In most custom controls where the cursor is placed at the front of the value, we have the option of a quick cmd-A in order to reset the value.

Since neither range selection or cmd-A are available to us in this context, I wonder about the value of placing the cursor at the end and allowing backspace keycode to remove the value and expand the menu?

I'm not entirely sure, but the action feels off atm. It's not intuitive when I see the front-positioned blinking cursor that I can't interact with the value via keyboard. I wonder if a more complete demo (mocking up additional form fields) would be illuminating?

@allisonacs
Copy link

Since neither range selection or cmd-A are available to us in this context, I wonder about the value of placing the cursor at the end and allowing backspace keycode to remove the value and expand the menu? I'm not entirely sure, but the action feels off atm. It's not intuitive when I see the front-positioned blinking cursor that I can't interact with the value via keyboard. I wonder if a more complete demo (mocking up additional form fields) would be illuminating?

I'd be happy to look at a more fully formed demo. From the way the constraints were explained to me, what Austin and I worked out felt like a reasonable compromise between expected behavior and technical feasibility, but I agree it's still a little awk.

Placing the cursor at the end and only initiating a replacement/menu via a backspace is… weird also. What if I attempt to type more in addition to the current string? It would just disappear, leaving only the newly typed thing—but there'd be no indication that that's what would happen. If we prevent you from typing when there's a selected item, that's also odd—it would feel broken.

But yea, I'm happy to feel this out with a more comprehensive demo.

@jzempel
Copy link
Member

jzempel commented Dec 17, 2018

yeah @allisonacs that backspace idea is clearly not viable. I'm wondering about our ability to detect a String vs an Object here? If it's possible, we could range select on a String (just like a native <input> element) and we could focus (no cursor?) on an Object (just like a native <select> dropdown).

I'm also wondering about using Object autocomplete as a justification for the design. Shouldn't an object-style dropdown (i.e. emoji picker) just be handled via our <Select> component? Why is visible text entry interesting for that case? Plz enlighten me 😉

@allisonacs
Copy link

yeah @allisonacs that backspace idea is clearly not viable. I'm wondering about our ability to detect a String vs an Object here? If it's possible, we could range select on a String (just like a native <input> element) and we could focus (no cursor?) on an Object (just like a native <select> dropdown).

I … think this would work, just need to see it?

I'm also wondering about using Object autocomplete as a justification for the design. Shouldn't an object-style dropdown (i.e. emoji picker) just be handled via our <Select> component? Why is visible text entry interesting for that case? Plz enlighten me 😉

Esp. in the case of an emoji picker, it's nice to be able to filter by typing, for example, "smile" to narrow down the emojis to all the emojis whose descriptions include "smile." Same kind of thing you see if you use the native apple emoji picker (cmd + ctrl + space). Another example would be showing avatars instead of people's names (though I think in that case people should be using the multi-select in order to show a tag).

@austingreendev
Copy link
Contributor Author

@jzempel @allisonacs Lets all have an offline discussion with some of this interaction today. Feels like we're going down a path that might require a separate PR rather than trying to wrap everything into this.

@austingreendev
Copy link
Contributor Author

@jzempel @allisonacs @ryanseddon This should get ready for a final review now. From our offline conversation with design, there is no longer a visual, input focus indicator when a user focuses the autocomplete with a keyboard. This now aligns with the current <Select /> styling 😄

Copy link

@allisonacs allisonacs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the example to remove the placeholder text.

Copy link
Member

@jzempel jzempel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice – I dig it!

Copy link
Member

@jzempel jzempel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, please update the WARNING text on the demo page, which currently indicates there is no Autocomplete visual component.

@austingreendev
Copy link
Contributor Author

Oops, please update the WARNING text on the demo page, which currently indicates there is no Autocomplete visual component.

Updated!

Copy link
Member

@jzempel jzempel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@ryanseddon
Copy link
Contributor

image

Adding a message prop the spacing looks off, also if I set the validation of the autocomplete that should filter through to the message too correct?

@austingreendev
Copy link
Contributor Author

Adding a message prop the spacing looks off, also if I set the validation of the autocomplete that should filter through to the message too correct?

@ryanseddon This is now corrected. Thanks for catching that.

@austingreendev austingreendev force-pushed the agreen/autocomplete-component branch from 7f8df67 to 7c2001d Compare December 21, 2018 19:28
@austingreendev austingreendev merged commit f53f34d into master Dec 21, 2018
@austingreendev austingreendev deleted the agreen/autocomplete-component branch December 21, 2018 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

6 participants