Public API: The Basics

[CMTegner]

tl;dr: We want to make Autocomplete's public API better, and we need your input!

Current API, for reference:

value: any,
onChange: func,
onSelect: func,
shouldItemRender: func,
sortItems: func,
getItemValue: func.isRequired,
renderItem: func.isRequired,
renderMenu: func,
menuStyle: object,
inputProps: object,
wrapperProps: object,
wrapperStyle: object,
debug: bool,

Some thoughts on the current API:

1. The fact that we need to wrap the input in another element before returning it is an implementation detail (due to how React works) and should not be visible to the user. wrapperProps and wrapperStyle are the main culprits. There is a way to do away with the wrapper entirely - by rendering the menu as a new React tree - but that brings with it extra bookkeeping and logic that we may not want to have to deal with in the long run. It also prevents relative-positioning of the menu, which is only possible when the menu is a child of the same node as the <input>.
2. The user should be able to think of Autocomplete is an augmented <input> field, and use existing knowledge of the native input controls to quickly configure it. Passing value, onChange, className, placeholder, etc, should just work like it does on a <input> element. Currently the native <input> props are "hidden" behind inputProps, while only the Autocomplete-specific props are exposed as top-level props.
3. Similarly, the user should be able to expect Autocomplete to behave and react like a native <input> element. Calling instance.focus() should focus the input, and instance.select() should select any text in the input. onFocus and onBlur should be triggered accordingly, as well as any other native event. Arrow keys should navigate text, and return should submit any surrounding form. The consumer shouldn't have to consider tradeoffs when choosing Autocomplete, it should be purely additive.
4. shouldItemRender, sortItems, and how String#indexOf is used to determine matches in maybeAutoCompleteText should maybe not be of a concern to Autocomplete. We did a big improvement by moving value out of state, and I think we can further improve by handing more responsibility to the consumer.

In my mind Autocomplete is this:

An <input> field with typeahead support and the ability to render an accessible list of options with keyboard and mouse navigation.

I keep this in the back of my mind because it helps me focus on what Autocomplete should be doing above all else.

I suggest - to get started - that we pass all top-level props to the wrapped <input> (minus the ones that are Autocomplete specific, naturally), and thus removing inputProps. This also includes ensuring that any event handler passed in (onBlur, onClick, etc) are attached correctly, even if we use them internally.

Next, I suggest adding all instance methods which one would expect to find on a <input>, e.g. focus, select, blur, etc..

I'll leave this issue open for a bit before we start any work. Hopefully we can get a discussion going to ensure we're heading down the correct path.

[CMTegner]

/cc @sprjr

[whichsteveyp]

There is a way to do away with the wrapper entirely - by rendering the menu as a new React tree - but that brings with it extra bookkeeping and logic that we may not want to have to deal with in the long run.

Aye, I'm not even sure how to do this entirely. I think our goal should be to only render two things: 1) <input /> that is enhanced, and 2) a drop down of the items to select. I think the wrapper is something most composers would really rather have full control over. Really all we do is pass props for them any way. I'm wondering if it would be better to help users with this instead. Some ideas:

<Autocomplete wrapper={MyComponent} {...rest} />

<Autocomplete {...props}>
  <MyComponent />
</Autocomplete>

This might allow us to avoid div soup, giving users full control of the wrapper in which they'd like to render the enhanced input into. Because we need to render 2 things (and we can't return siblings) this could be controlled, or we could perhaps default it nicely (e.g. it might be cool to use a <label> as the default to address some of our other a11y issues out of the box?). Just thoughts, if using React.Children.only() or passing out children to a wrapper is more complex than the other tactic you mentioned above, I'd like to learn more about it.

The user should be able to think of Autocomplete is an augmented field, and use existing knowledge of the native input controls to quickly configure it. Passing value, onChange, className, placeholder, etc, should just work like it does on a element.

Agreed. The simpler our API, the better. It might even be nice if we identify the props required by Autocomplete that are not native, and only expose those in propTypes. The other native props that it might rely on, but are configurable by the composers, we could simply add to getDefaultProps() with sensible defaults?

shouldItemRender, sortItems, and how String#indexOf is used to determine matches in maybeAutoCompleteText should maybe not be of a concern to Autocomplete.

maybeAutoCompleteText I think (iirc) seems to bounce all over the internals, and then it turns in to a function that I can't override and makes the component unusable for a few of the use case I have where I'd rather just take the value entered and hand back a controlled list of the matches (e.g. - don't necessarily want to hand it all matches and have it filter them down)

I think that we've grown the props list by simply adding props to fix problems, and not thinking about fixing the problems and then adding props to help with that. I'm all aboard in terms of simplifying that list and making it easier to reason about and interact with for sure.

[carlpaten]

I just wanted to mention that, as a user of react-autocomplete, the issue description here has been incredibly useful!

[hoodsy]

@CMTegner maybe this should be in the README, I keep coming back here 😁

[pcalves]

What's the status of this issue? Any roadmap for the improvements detailed in the original message?

I'm currently developing an application and resorting to some extremely "hacky" workarounds because I can't set a custom onBlur as a prop. I'd be happy to help implement some of these changes as well — this component is already great in so many ways, it just needs that extra push. 😊

[whichsteveyp]

Hey Paulo, thanks for the offer to help out - more than happy to review a PR. Would rather that than you having to write very hacky code in your code.

Currently looking to ship @CMTegner's other PR soon - not sure if that behavior would help your situation out. For onBlur, is it not possible to pass it via inputProps any more? Not in front of the latest API currently so I could be wrong.

Feel free to send code this way if you'd like and we can get the polish/improve items shipped with you!

Sent from my iPhone

On Sep 1, 2016, at 7:33 AM, Paulo Coelho Alves notifications@github.com wrote:

What's the status of this issue? Any roadmap for the improvements detailed in the original message?

I'm currently developing an application and resorting to some extremely "hacky" workarounds because I can't set a custom onBlur as a prop. I'd be happy to help implement some of these changes as well — this component is already great in so many ways, it just needs that extra push. 😊

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[pcalves]

Thanks for the reply, @sprjr!

For onBlur, is it not possible to pass it via inputProps any more?

Looking at the current code, it doesn't seem possible:

<input
  {...this.props.inputProps}
  role="combobox"
  aria-autocomplete="list"
  autoComplete="off"
  ref="input"
  onFocus={this.handleInputFocus}
  onBlur={this.handleInputBlur}
  onChange={(event) => this.handleChange(event)}
  onKeyDown={(event) => this.handleKeyDown(event)}
  onKeyUp={(event) => this.handleKeyUp(event)}
  onClick={this.handleInputClick}
  value={this.props.value} />

If you pass onFocus, onBlur, onChange, etc. via inputProps they'll be ignored because the component is already setting those events, right? #54 already handled this issue as well as I can think of, but that PR has been closed because "it'll likely be superseded shortly by the work that will be spun off as a part of #138." So I'm guessing @CMTegner probably had some other ideas as for how to handle this issue?