Component autocomplete #971
Conversation
|
Just started reviewing :) |
Sent from 
There was a problem hiding this comment.
Hi @gcmznt !
This is such a difficult component, let's go step by step to make it awesome! 😄
First of all, don't know if you already saw this but here is the docs for Lexicon Autocomplete: https://docs.google.com/document/d/15r6t5Maiavg7ggxE8ulTATTuyqGVaq75yj9p-zfsQqU/edit
One of the things we need to implement is that result should be shown alphabetically ordered.
Also I added some comments to start improving the component.
Do not hesitate to ask anything. Thanks!
| label: Config.string(), | ||
| }; | ||
|
|
||
| ClayAutocomplete.STATE = { |
There was a problem hiding this comment.
Missing JSDocs in all state
| }; | ||
|
|
||
| ClayAutocomplete.STATE = { | ||
| dataSource: Config.oneOfType([ |
There was a problem hiding this comment.
To be consistent with other components such as Charts this should be named data. It should also accept an Array of Strings and a function. See https://github.com/liferay/clay/blob/master/packages/clay-charts/src/ChartBase.js#L4
| Config.arrayOf(Config.shapeOf(itemShape)), | ||
| Config.string(), | ||
| ]), | ||
| suggestions: Config.arrayOf(Config.shapeOf(itemShape)).value([]), |
There was a problem hiding this comment.
All params that are meant to be used internally and not passed from the outside must be marked as internal .internal() and the name has to be preceded by _
| elementClasses: Config.string(), | ||
| hasFocus: Config.bool().value(false), | ||
| id: Config.string(), | ||
| query: Config.string().value(''), |
There was a problem hiding this comment.
query should be always the same value of value, so I think we can get rid of this param
| data-onblur="{$_handleBlur}" | ||
| data-onkeydown="{$_handleKeydown}" | ||
| > | ||
| <ul {$listAttributes}> |
There was a problem hiding this comment.
Can't we use ClayDropdown here?
| created() { | ||
| this.provider = new DataProvider(this.dataSource, { | ||
| elements: this.maxSuggestions, | ||
| paramName: 'query', |
There was a problem hiding this comment.
This should be configurable so ClayAutocomplete should receive a paramName param too.
| .filter(query, data, { | ||
| // pre: '<strong>', | ||
| // post: '</strong>', | ||
| extract: el => el.label, |
There was a problem hiding this comment.
Which object property we want to extract should be configurable.
| } | ||
|
|
||
| function getDataFromURL(query, url, paramName = 'q', cb) { | ||
| fetch(`${url}?${paramName}=${query}`) |
There was a problem hiding this comment.
We need to manage all kind of urls, for example, what if the passed url already has queryParams? You can use metal-uri https://github.com/metal/metal-plugins/tree/master/packages/metal-uri to deal with this.
|
Just started reviewing :) |
|
Superseeded for now by #1292 (although we might want to take a step and extract the autocomplete feature as a standalone) |
This is the first implementation of the autocomplete text field