feat(autocomplete): implement basic shared DOM #6709
Conversation
aymeric-giraudet
requested review from
a team,
dhayab and
shaejaz
and removed request for
a team
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 6aecadb:
|
|
|
||
| return ( | ||
| <AutocompleteIndex | ||
| // @ts-expect-error - there seems to be problems with React.ComponentType and this, but it's actually correct |
There was a problem hiding this comment.
should we cast it instead?
| <AutocompletePanel isOpen={isOpen}> | ||
| {indices.map((index) => ( | ||
| <Index key={index.indexName} indexName={index.indexName}> | ||
| <AutocompleteIndexComponent itemComponent={index.itemComponent} /> |
There was a problem hiding this comment.
ok for now, but later once we do keyboard, we likely will want it to be a flat list of indices and use connectAutocomplete? not sure tbh but it likely doesn't change much
| export * from './chat/ChatHeader'; | ||
| export * from './chat/types'; | ||
| export * from './autocomplete'; |
There was a problem hiding this comment.
not consistent between chat (no barrel file) and autocomplete (barrel file). I don't have a specific preference, but we'll want to consolidate after both sprints are over.
| expect(screen.getByRole('searchbox')).toBeInTheDocument(); | ||
| await screen.findByText('Item 1'); | ||
|
|
||
| expect(container.firstChild).toMatchInlineSnapshot(` |
There was a problem hiding this comment.
I think a different snapshot for the search box and a separate one for the panel may make sense?
|
|
||
| return ( | ||
| <div className={cx('ais-AutocompleteIndex', classNames.root)}> | ||
| <ul className={cx('ais-AutocompleteIndexList', classNames.list)}> |
There was a problem hiding this comment.
in hits we use ol, so we should here too.
|
|
||
| return ( | ||
| <div className={cx('ais-AutocompleteIndex', classNames.root)}> | ||
| <ul className={cx('ais-AutocompleteIndexList', classNames.list)}> |
There was a problem hiding this comment.
I think there probably (later) should be only one list, as you're navigating across all lists with the keyboard. However there's an argument to be made for separate lists for each index, as that allows us to have header/footer per index.
| aria-expanded={isOpen} | ||
| aria-haspopup="listbox" | ||
| > | ||
| {children} |
There was a problem hiding this comment.
in a later iteration (when we do the final dom) we likely want to have the search box be part of this too, and then all the intermediate components also be part of the autocomplete. For now this is a logical split.
Summary
FX-3496
Result
Was not too sure how to split components effectively, for now I did :
Autocomplete: the wrapper itself, has accessibility attributes taken from the autocomplete library DOMAutocompletePanel: same DOM as for autocompleteAutocompleteIndex: meant for more generic indices, we'll have something special for query suggestions and recent searchesThis is still using the internal searchbox implem, not sure if we'll want a custom one or not.