Skip to content

Conversation

@austingreendev
Copy link
Contributor

@austingreendev austingreendev commented Jan 10, 2019

Description

This PR introduces a new high-abstraction element, Multiselect. This implementation is very similar to Autocomplete, but with the original tweaks present in the AutocompleteContainer example.

Detail

2019-01-10 14-23-20 2019-01-10 14_30_04

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

@coveralls
Copy link

coveralls commented Jan 10, 2019

Coverage Status

Coverage decreased (-0.4%) to 93.51% when pulling c9c6b13 on agreen/multiselect-component into 3b15875 on master.

@allisonacs
Copy link

do you have this somewhere i can play with it?

@austingreendev
Copy link
Contributor Author

do you have this somewhere i can play with it?

@allisonacs I've pre-published to https://garden.zendesk.com/react-components/autocomplete/#multiselect it should be the same interaction as the original example built with the render-prop container.

@allisonacs
Copy link

IMO, there's an odd interaction in the keyboard nav. If i use the arrow keys to move back through the list (indicated by focus), and then delete a tag, the cursor returns to the end of the input and starts flashing again. Instead, I'd expect it to focus on the tag prior to the left of the item just deleted.

https://cl.ly/0ce3045bab23

@jzempel
Copy link
Member

jzempel commented Jan 11, 2019

Agreed with @allisonacs (or focus right, depending on RTL).

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.

This is coming together nicely, @Austin94! Along with Allison's "delete" comment, can you add an example for what it feels like to have a MultiSelect that can add values dynamically? I.e. if I type foo (followed by comma/enter), then that is converted to a tag and added to the list.

@jzempel
Copy link
Member

jzempel commented Jan 16, 2019

Please add support here for #253

@austingreendev
Copy link
Contributor Author

Please add support here for #253

@jzempel with this being a high-abstraction component would this be necessary? Users aren't able to combine this with a ButtonGroup or it's equivalent.

@jzempel
Copy link
Member

jzempel commented Feb 12, 2019

Yes, we want the API to be consistent for all fields. Also, this gets us away from pre-imagining all valid use cases – it's not always about combining with a button. Sometimes there are other space constraints where the focus treatment should appear internal to the field container.

@allisonacs
Copy link

@Austin94 I've been chatting with a few folks and came across a use case where the component needs to be able to show a selection/tag which is in error. I can't remember if we had that in the old components—I feel like we must have—but if we don't, let's make sure we get that functionality in here as well (it will be necessary alongside the "add" capability mentioned by JZ above).

@austingreendev
Copy link
Contributor Author

@jzempel @ryanseddon @allisonacs This is now at a place that matches the original functionality provided in the examples that used the AutocompleteContainer. (I can run locally for you to re-evaluate)

From all of the conversations over the past few weeks, it's obvious that this style of API doesn't fit every possible use case. But, as an intermediary step, this PR should help those that are stuck with the most common implementations.

Some of this ultra-flexibility, like Tag validation, async indicators, and paginated scrolling, should be addressed in a different API/approach that is more flexible than the path we have gone down with this package.

@austingreendev austingreendev requested a review from jzempel March 11, 2019 21:11
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.

🎉

@austingreendev austingreendev merged commit e0d863d into master Mar 12, 2019
@austingreendev austingreendev deleted the agreen/multiselect-component branch March 12, 2019 17:12
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