Dropdown enhancements: Better remote API support, item customization

[jeffcarbs]

This wound up a little long, sorry in advance!

I'm trying to use the dropdown with a remote data source. I'm running into a few interconnected issues so I'm just going to enumerate them here:

1. Disabling/customizing local filtering

I'm using the onSearchChange callback to send a request to the server with the search term and it's sending me back filtered results. The issue I'm running into is that the results are being filtered client-side as well:
https://github.com/TechnologyAdvice/stardust/blob/master/src/modules/Dropdown/Dropdown.js#L527-L531

// filter by search query
if (search && searchQuery) {
  const re = new RegExp(_.escapeRegExp(searchQuery), 'i')
  filteredOptions = _.filter(filteredOptions, (opt) => re.test(opt.text))
}

In my case, the text label for each option is not the criteria that's used server-side, causing results to be filtered out incorrectly. I'd basically just like to skip this filtering since I'm relying on the server to do it.

Some possible solutions:

1. Allow disabling local filtering via a boolean
2. Allow someone to pass as a prop their own function that gets passed to _.filter.
   • In my case I could just pass () => true.
   • I could also see it being useful to perform a more complex match based on the option or match based on some criteria other than text.
3. Allow someone to pass as a prop their own filter function that takes the options and the search query and returns filtered options. Similar to 2, but 2 wouldn't allow you to pre-calc a regex like you're doing above for performance.

Option 2/3 seems to provide more flexibility. The default could be the current functionality since that's what one would expect for the simple text-option case.

1a. Hiding/showing the "Add item" option

There' also a filter function to determine whether the "Add item" option should be shown that's slightly different than the filter above:

// insert the "add" item
if (allowAdditions && search && searchQuery && !_.some(filteredOptions, { text: searchQuery })) {

That could also be a prop (e.g. showAdditionOption or some better name) that's a function that takes the options and the search query and returns a boolean. That would be necessary for my case, but you could imagine using that to doing things like validation or more complex logic.

2. Option customization

I want to display a more complex "dropdown item" with some custom styles. I think this could be done with two small (backwards compatible) tweaks:

1. Allow specifying the element type of Dropdown.Item as a prop so one could pass a custom component. This is already in progress: Breaking Changes: Add component augmentation #414
2. Instead of each object in the options array only having value and text keys, treat the object as props to be passed to the object. E.g. Change the snippet here to something like:

return _.map(options, (opt, i) => (
  <DropdownItem
    key={`${opt.value}-${i}`}
    active={isActive(opt.value)}
    onClick={this.handleItemClick}
    selected={selectedIndex === i}
    {...opt}
  />
))

I tried just rendering my own menu+items but this breaks the keyboard up/down/enter functionality, which relies on an options array. That could be something to address in addition or instead of the above.

3. Hiding the dropdown caret

This is pretty self-explanatory and might even be possible now - if so, how? Passing undefined as the icon property does the trick :)

4. Customize "no results" text

Add a prop like noResultsLabel analogous to additionLabel?

[levithomason]

1. Disabling/customizing local filtering

...the text label for each option is not the criteria that's used server-side...

Hm, this might be a use case for the Search module. If there are no options in the list or if searches are potentially unrelated to the readable items, I think it is probably just a search field.

I think the purpose of the Dropdown is to show the user a list of options. When searching, they are trimming down the list that is there. With the Dropdown being complex as it is, and the fact that there is a dedicated Search component (awaiting PR, #195), I think these features are best implemented there.

Give the search a go on the SUI docs and let me know if you think this more closely suits your use case.

1. Allow disabling local filtering via a boolean

Filtering can be disabled by removing the search prop. Is this use case different than removing the search prop? If so, what does a searchable Dropdown do when you search if it doesn't remove the non-matching items?

1. Allow someone to pass as a prop their own filter function that takes the options and the search query and returns filtered options...

This sounds like a good improvement, I'd merge a PR implementing this. Perhaps searchFilter?

1a. Hiding/showing the "Add item" option

Could you give a little more info? Does your use case require showing the "Add item" option but with different logic?

...could also be a prop (e.g. showAdditionOption or some better name) that's a function that takes the options and the search query and returns a boolean...

Yep, would take a PR for this as well.

2. Option customization

Yep, we pulled back on support for defining your own markup here. The reasoning was that there is no reliable way to "get items" from children. There would need to be some item register mechanism in place for this to work well I believe.

1. ...Allow specifying the element type...

In progress as you noted, #414.

1. ...treat the object as props to be passed to the object....

Would take a PR for this as well. It is the way our more recent components work, too. This should really be library wide. Arrays of items should always spread as props on the children they generate.

3. Hiding the dropdown caret

Glad it worked. Was this a selection Dropdown or default (menu) style dropdown?

4. Customize "no results" text

Merged, thanks!

---

EDIT

Added missing 1a response.

[levithomason]

I believe 2, 3, 4 are now addressed, could you confirm? Now, just to resolve 1 and 1a.

[venelin-mihaylov]

+1 for option to disable local filtering, or at least customize it. Custom filtering is not against the nature of the component, IMHO.
Would you accept a PR for adding such an option as prop to Dropdown?

I want to use the dropdown component in a similar fashion to @jcarbo .

I tried creating a simple CountrySelect component.
The component options: "flag - country name"
The user can enter search text to lookup a country.
No luck :( .

1. Dropdown item accepts a iconName prop, but flags are not Icon(s)
2. The local filtering breaks search, if text is a react element, not clear text.

[levithomason]

Could you post some code snippets with the expected behavior for each? What you are describing sounds like features already supported. There are known limitations elsewhere, but I'm seeing those described in this issue.

The Dropdown needs more support for the sub component usage. I'd love to add more support with doc site examples. Though, I think I can achieve what I think is being described in this particular issue with the existing component.

Disable Search

Search (local filtering) is enabled by the search prop. To disabled it, simply omit it or set it to false.

<Dropdown options={...} />
// or 
<Dropdown options={...} search={false} />

Custom Items & Custom Search

When using sub components, you are using the controlled component pattern. So you must handle search change, value, and item clicks manually. There is no reliable way (outside of using context) to add event listeners to the child items and filter them for you. This also means you can customize the search however you choose. Here, I've just used a simple "contains" RegEx search.

Again, there are issues with this usage (such as keyboard navigation and propType warnings) and improvements that could be made (using value over text to set the displayed value). Though, I am able to create a working country search selection Dropdown with Flags.

const flagOptions = Flag._meta.props.name.map(name => ({
  text: _.startCase(name), value: name,
}))

export default class CountrySearch extends Component {
  componentWillMount() {
    this.setState({ flagItems: this.filterItems(''), value: '' })
  }

  filterItems = (query) => flagOptions
    .filter(({ text }) => new RegExp(query, 'gi').test(text))
    .map(({ text, value }) => (
      <Dropdown.Item key={value} value={value} onClick={this.handleItemClick}>
        <Flag name={value} /> {text}
      </Dropdown.Item>
    ))

  handleItemClick = (e, value) => this.setState({
    value: _.find(flagOptions, { value }).text,
  })

  handleSearchChange = (e, query) => {
    const flagItems = this.filterItems(query)
    this.setState({ flagItems })
  }

  render() {
    const { flagItems, value } = this.state

    return (
      <Dropdown fluid search selection text={value} onSearchChange={this.handleSearchChange}>
        <Dropdown.Menu>
          {flagItems}
        </Dropdown.Menu>
      </Dropdown>
    )
  }
}

Please let me know your exact use case as I would like to add more support to the Dropdown.

[levithomason]

I should note, if you can live without the Flag, you can still customize the search and things are far more simple. This is the current intended use of the Dropdown (or Select), so you also get full keyboard nav and no limitations.

Here I search by "starts with":

const flagOptions = Flag._meta.props.name.map(name => ({
  text: _.startCase(name), value: name,
}))

export default class CountrySearch extends Component {
  state = { options: flagOptions }

  handleSearchChange = (e, query) => {
    this.setState({
      options: flagOptions.filter(({ text }) => {
        return text.toLowerCase().startsWith(query.toLowerCase())
      }),
    })
  }

  render() {
    const { options } = this.state

    return (
      <Select fluid search options={options} onSearchChange={this.handleSearchChange} />
    )
  }
}

[venelin-mihaylov]

@levithomason Thank you very much for the examples and your time.
It is true, that one can use the "controlled component" pattern and manually control the props of the dropdown. However, IMHO this approach is sub-optimal.
Your example for the country with flag is missing keyboard navigation. Adding it in to a parent component to Dropdown would be additional work / would add complexity / would miss props validations / etc.
A simple approach - requiring less work on the user of the component - would be to provide a "filter" prop. Or at least an option to disable the built-in filtering.
(Material-UI provides such a hook: http://www.material-ui.com/#/components/auto-complete , filter prop)
With this approach you do not miss any of the functionality of the dropdown and get additional customization hook.
Maybe I am missing something, and also I am fairly new to React, @levithomason please let me know what do you think.

Example of Dropdown with filter prop:
https://gist.github.com/venelin-mihaylov/43f5d75c1eda7dbf9a3035949e334825
Example usage for country select with flag
https://gist.github.com/venelin-mihaylov/64773135ddc58535d93a15d8701ae994

[dvdzkwsk]

@venelin-mihaylov as @levithomason pointed out, filtering can, without any changes to Stardust as it exists today, be disabled by disabling the search prop on the Dropdown component.

// implict
<Dropdown options={...} />
// explicit
<Dropdown options={...} search={false} />

As far as the filter customization, that definitely sounds like a worthwhile solution. Here are my two proposals. Note that these are not mutually exclusive.

Internal Filtering

<Dropdown search={myFilterFunction} />

This provides an easy way to extend a Dropdown that is already search capable into one that uses a custom filter function. Instead of just passing true, you can pass a function that will filter the items internally. This means that if you provide a set of options to the Dropdown instance, only a subset of them will be displayed depending on the result of the filter function and state of the search input. While intuitive, it strays from traditional React paradigms since you are no longer accurately describing which options to render in one place. It is, however, incredibly convenient.

External Filtering

This is the more traditional React way of doing filtering, and one that I'm in support of because it pulls state upwards and away from child components, allowing you to store more meaningful state toward the top of the application. With this approach there is definitely more manual wiring to do, but it's a worthwhile alternative and not one that should be dismissed just because it's syntactically lengthier. This is the same exact idea that @levithomason had earlier.

_onSearchChange = (e, query) => {
  this.setState({ filteredOptions: someItems.filter(...) })
}

render () {
  const options = this.state.filteredOptions.map(...) // create elements

  return (
     <Dropdown search selection onSearchChange={this._onSearchChange}>
        <Dropdown.Menu>
          {options}
        </Dropdown.Menu>
      </Dropdown>
  )
}

The distinction between onSearchChange and search={searchFunction} is the effect that it has on the options. onSearchChange is a simple event, it does not actually affect the options in any way; this is exactly the same as inputs work with onChange, for example. You become responsible for making the change yourself, which clearly provides more control in userland. search={searchFunction} on the other hand, is a shorthand way of telling the Dropdown how to filter the items itself. This removes more boilerplate for the user, and will be useful for prototyping or simpler use cases.

I believe these two options are not mutually exclusive, but if I had to pick one I'd go with the latter due to correctness and extensibility.

[venelin-mihaylov]

If I can pass a function as a search prop, not just bool, that would be just what I need, thank you very much!

As far as I can see, the internal filtering is done in Dropdown.getMenuOptions()
I just checked the source again, seems search prop is used only as a flag, i.e. not used as a function?
Search prop is documented only as bool.
Can you please comment on that?

The disadvantage of the external search for a country+flag example component is that the keyboard support is not working (and I do not know how to add it back to the controlled component, can it be done relatively easy?)

I need not disable search altogether, but customize the internal filtering (disable it in some cases) and still have search prompt and searchText

My use case:
Internal search:
Country with flag, search by country name, have keyboard support (tab, arrows, enter)
External search:

1. Have a dropdown with a server provided options.
   1.0 when the dropdown is expanded, the options need to fancy render themselves, e.g. a whole ContactCard, not only firstName-lastName
   1.1. Keyboard navigation should work. (tab, arrows, enter)
   1.2 The user types in search criteria
   1.3 The search is done on the server
   1.4 Only some options are sent to the client
   1.5 Options are stored in redux store
   1.6 Dropdown is subscribed to redux, receives prop options on redux store update
   1.7 When the user selects an option, a text is shown in the Dropdown to signify the new selection.

[dvdzkwsk]

@venelin-mihaylov you are correct. Currently, search can only be either disabled or enabled, it does not yet support a custom function. The search={someFunc} is my API proposal for supporting for your use case, and it sounds like you're in support of it.

We also discussed supporting keyboard navigation for more advanced dropdown item components by allowing users to pass in an array of React elements, rather than POJOS (Plain Old JavaScript Objects). So, if both of these were to be implemented, it would seem as though the needs delineated by this issue would be resolved :).

[venelin-mihaylov]

search={func} sounds good to me, looks easy to implement as well. Would you accept a PR? Or you will implement it?

Keyboard navigation is very nice to have ..., thank you for considering it.

[levithomason]

I'd accept a PR for the search prop, however, this will not solve either of @venelin-mihaylov's use cases since they both require custom sub component items and search with keyboard navigation. The Dropdown can only handle search behavior and keyboard navigation when using the options prop, not sub components.

@venelin-mihaylov has specified:

Internal search

Country with flag, search by country name, have keyboard support (tab, arrows, enter)

Having a flag means using custom <Dropdown.Item /> sub components. This negates search and keyboard navigation. Search requires an array of options to filter prior to render. Keyboard navigation requires an options array to track the selectedIndex.

Be aware that adding a search function will do nothing for this use case.

External search

the options need to fancy render themselves, e.g. a whole ContactCard, not only firstName-lastName

Again, this requires custom <Dropdown.Item /> sub components which negates the search functionality and keyboard navigation.

The search is done on the server

This requirement is what makes this a Search component (#424) and not a Dropdown. A selection Dropdown is for selecting from a list of options. The Dropdown "search" is only a convenience for quickly finding the item in the list of options. You cannot search the options while also not filtering the items.

When a user "searches" for something using a Search (#424) they see a loading state and eventually receive a hard list of the top results. A Search does not allow for filtering the local results. Instead, there are a limited number of "maxResults". If the user needs to "filter the list", they would perform a new search and receives new results, replacing the items in the list.

I'll propose a solution anyhow, since I think in React the paradigm is a little different and may support both use cases.

Proposal

Custom Dropdown Search

Let's add @davezuko's proposed search={myFilter} prop. This will allow customizing a local filter only. It does not account for searching on the server.

Search and Keyboard Nav with Children

We need to be able to reliably loop the children in order to provide this. I propose we either:

1. Accept an array of item elements (i.e. <Dropdown.Item />s)

   const options = [
     <Dropdown.Item key='us' value='usa'>
       <Flag name='us' />
       United States of America
     </Dropdown.Item>,
     // ...
   ]
   
   <Dropdown options={options} />

2. Provide an itemRenderer={this.renderItem} prop which would be passed each option and be expected to return a <Dropdown.Item />. This is identical to how we do Table cells and headers with cellRenderer and headerRenderer props.

   const options = [{ text: 'United States of America', value: 'usa' }, ...{}]
   
   const renderItem = (option) => (
     <Dropdown.Item value={option.value}>
       <Flag value={option.value} />
       {option.text}
     </Dropdown.Item>,
   )
   
   <Dropdown options={options} itemRenderer={renderItem} />

Server Side Searching

This could be accomplished by extending the proposed search API to allow both enabling search and disabling the search filter function. This sounds confusing and I think the API shows that as well. If search enables searching and the value is the filter function then enabling search, disabling the search filter, and capturing search queries it might look like so:

<Dropdown
  search={null}
  onSearchChange={this.handleSearch}
  options={this.state.options}
/>

Compare this to:

<Search
  onChange={this.handleSearch}
  results={this.state.results}
/>

I agree with SUI in the separation here. A Search should never filter the list, but only replace the results on query change. The search selection Dropdown should always filter the list on query change. The Dropdown is just a modern <select /> component. The user flow for a traditional server search is not compatible with the user flow of using a select component.

[dvdzkwsk]

FYI I did point out:

We also discussed supporting keyboard navigation for more advanced dropdown item components by allowing users to pass in an array of React elements, rather than POJOS (Plain Old JavaScript Objects)

Which falls in line with your proposal to allow options to be an array of elements. I'm definitely a huge 👍 for this over itemRenderer, which would require more work interacting with the component API than requiring the user to map them into elements themselves. This is due to the fact that they'd have to pass two props, the renderer and the list of objects, rather than just the elements. The separate renderer prop also limits the ability for the component rendering the dropdown to receive an array of elements, if that need should ever arise.

[levithomason]

Indeed, I should have specified the array of elements was per your offline suggestion. It is also inline with where we are going with all shorthand props. Shorthand is used to generate children. Shorthand props then should accept a primitive that gets mapped to the most intuitive child prop, a props object used to createElement, or an instance which is cloned.

So, we can implement search={myFilter} and options={/* string[]|object[]|element[] */}. However, I'm still not convinced that server side searching (enabling search and simultaneously disabling menu search) belongs in the Dropdown component.

[venelin-mihaylov]

I am currently using the children prop of options to fancy render the options ... with that I can custom render options (pass react elements there) and also have keyboard nav.
https://gist.github.com/venelin-mihaylov/64773135ddc58535d93a15d8701ae994
@levithomason Isn't that ok?

(The only hindrance is that {text} is displayed always first, which is not always appropriate, but's that's a minor thing ...)

[levithomason]

This is good. Though this is using custom items, it is not doing so via JSX in the Dropdown children. It is still using the options prop with an array of props objects, which is what we plan to officially support for all shorthand. This is the Dropdown children usage that does not work with search and keyboard nav:

<Dropdown>
  <Dropdown.Menu>
    <Dropdown. Item {...} />
  </Dropdown.Menu>
</Dropdown>

Though the provided example is working, it is not supported usage and will not work in v1. The reason is that children cannot be used with shorthand props in v1. The text prop is shorthand:

<Dropdown.Item text='USA' />
// vs
<Dropdown.Item>USA</Dropdown.Item>

I'd recommend refactoring your map function to use only children. This way it is v1 compliant and also you can control the ordering of the text/flag.

const options = countries.all
  .filter(c => c.status === 'assigned' && Flag._meta.props.name.indexOf(c.alpha2.toLowerCase()) !== -1) // eslint-disable-line no-underscore-dangle
  .map(c => ({
    value: c.alpha2.toLowerCase(),
    name: c.name,
-   text: c.name,
-   children: <Flag name={c.alpha2.toLowerCase()} style={{marginLeft: 5}} />
+   children: (
+     <div>
+       <Flag name={c.alpha2.toLowerCase()} style={{marginLeft: 5}} /> {c.name}
+     </div>
+   )
  }))

Lastly, I'd remove usage of the _meta prop. The underscore means this is a private property. We may change it or remove it entirely at any point with a patch version bump and your app will break. I also do hope to remove it entirely, someday.

[levithomason]

I've added issues asking for PRs for the following:

• https://github.com/TechnologyAdvice/stardust/issues/469 support custom search function
• https://github.com/TechnologyAdvice/stardust/issues/470 support an array of string|element options

[levithomason]

@jcarbo I believe all issues raised in the OP are now solved. Could you confirm or expound on what's left in this issue? Thanks much.