liferay / clay

A web implementation of the Lexicon Experience Language
http://clayui.com
Other
206 stars 470 forks source link

Verify clay-autocomplete for 3.0.0 release #2444

Closed bryceosterhaus closed 4 years ago

bryceosterhaus commented 5 years ago

Additional Questions:

markocikos commented 5 years ago

@bryceosterhaus please assign this to me.

markocikos commented 5 years ago
  • [ ] Are you able to easily implement a demo of this component? Are there any “gotchas” when implementing? Provide a link to codesandbox for a demo.

I was able to implement basic example, but it was not easy. It is poorly documented and the API has many unnecessary and unintuitive requirements (more on this later).

I was not able to implement async example, I could not figure out how to import / enable useResource.

  • [ ] Does the component accept additional HTML attributes? For example, adding an aria attribute to a button.

<ClayAutocomplete>, <ClayAutocomplete.Input> and <ClayDropDown.ItemList> accept attributes. <ClayAutocomplete.Item> sets attribute to span in li, not sure if that is acceptable. <ClayAutocomplete.DropDown> does not accept attributes.

  • [ ] Does component accept a ref? Is the ref value what you would expect?

No? I am getting an error where ever I try to place it, I may be doing something wrong.

  • [ ] Does this component allow for different translations?

Yes. Only placeholder text would require translation, and that is a part of API.

  • [ ] If this is a form element, does it accept a name attribute?

Yes. <ClayAutocomplete.Input>, relevant for forms, accepts the name attribute.

  • [ ] Were you able to find documentation for this component? Was it clear?

I found it, but is is not clear. The documentation is inadequate and incorrect, with partial and poorly useful examples:

Additional Questions:

  • Is there anything about the public API that you would change?

Yes. There are many required parts that are unnecessary and can be hidden behind defaults. For example, why must I set match={value} for each item? It is autocomplete, of course I want to match what I type in with items, this will be the case for 99.99% of the cases. Why must I explicitly set when the dropdown opens? I almost always want to it to open when I start typing. For odd fringe cases it is ok to have these as possible overrides in API.

I added to my basic implementation, in a comment block, what I would consider a good basic example. All the rest I would hide behind defaults.

I would also expect these behaviors out-of-the-box:

  1. When you click on an option, it should copy to input. This is implemented in storybook examples.
  2. The keyboard functionality, the second example in storybook, should be default.
  3. When you click on an option, it should close dropdown menu.
  • Is the naming of the available props clear?

Yes.

matuzalemsteles commented 5 years ago

hey @markocikos thanks for your sincerity about this component maybe it has given you a different idea but this component is a low level which means that it is controlled, broken into small parts and does not deal with internal logic such as opening the DropDown, close and etc... Much of it is really manual to cover other use cases, this allows you to have full control of it. This allows us to reuse it in MultiSelect and allows people to easily customize DropDown content and add their own filter rules...

When you want to create something simpler, you might want to use ClayInputWithAutocomplete (we still lack documentation and we will work on them in the coming days) which deals with all logic and does not have many things to set up.

About documentation it can really be a glitch that doesn't pass this information right, the idea there was to describe the small components of Autocomplete and tell what they do and demonstrate the composition.

Regarding the use of async mode, with DataProvider I think it is also a flaw in our documentation that can be improved by linking the reader to the component page but also does not prevent you from using any other feature to fetch your data... it's a large composition in this component and increases the learning curve.

We don't add imports to the example because they are React Live which allows you to edit for testing. We can add the commented imports, would that help you? what do you think?

I'm sorry that this created a headache for you 😞.

matuzalemsteles commented 4 years ago

@markocikos I'm closing this issue, but I just want to say that your opinions on this component will not be lost and we are looking to improve... we made the release last week so let's test with the teams and see what works what doesn't work and add improvements in future releases.

I added code snippet imports in a recent PR (#2549) and we will release a version of Autocomplete with the high-level component (#2552).