<Collection> improvements: isPagination/isSearchable #507
Conversation
hvergara
force-pushed
the
collection-improvements
branch
from
71762eb to
ab5bca1
Compare
hvergara
added
the
pending-review
|
|
||
| TBD | ||
| Collections can also be filtered, adding a `isSearchable` property. Pass a custom `searchFilter` function to enhance your search experience. |
There was a problem hiding this comment.
Should we explain what the default search function is, or do they have to provide a search function?
| return ( | ||
| <Flex direction={direction} className={className} {...rest}> | ||
| {Array.isArray(items) && items.map(children)} | ||
| <Flex direction="column"> |
There was a problem hiding this comment.
These style props should be moved to CSS so they aren't render time styling. Should also provide a class name and take in a class name for customers to hook into.
There was a problem hiding this comment.
good idea, will add in next commit
| {Array.isArray(items) && items.map(children)} | ||
| <Flex direction="column"> | ||
| {isSearchable && ( | ||
| <Flex direction="row" justifyContent="center"> |
There was a problem hiding this comment.
Same comment as above on moving style props styling to CSS file.
There was a problem hiding this comment.
done in next revision
| {isSearchable && ( | ||
| <Flex direction="row" justifyContent="center"> | ||
| <SearchField | ||
| size="small" |
There was a problem hiding this comment.
Should this be customizable by the user? What if the customer wants a different size or variation?
There was a problem hiding this comment.
removed in next revision
| <Flex direction="row" justifyContent="center"> | ||
| <SearchField | ||
| size="small" | ||
| label="Search" |
There was a problem hiding this comment.
This string should be in the i18n file so we don't have to go looking for it later.
There was a problem hiding this comment.
done in next revision
| @@ -14,6 +14,7 @@ export const Pagination: Primitive<PaginationProps, 'nav'> = ({ | |||
| onNext, | |||
| onPrevious, | |||
| onChange, | |||
| role = 'navigation', | |||
| ...rest | |||
| }) => { | |||
There was a problem hiding this comment.
Isn't this already the implicit role in a nav element?
There was a problem hiding this comment.
true, will remove in next commit
| } | ||
| if (key === ENTER_KEY) { | ||
| onClearHandler(); | ||
| } else if (key === ENTER_KEY) { |
There was a problem hiding this comment.
Why else if? What's wrong with just if?
There was a problem hiding this comment.
key cannot be ENTER_KEY and ESCAPE at the same time
| @@ -68,13 +77,16 @@ export const SearchField: Primitive<SearchFieldProps, 'input'> = ({ | |||
| className, | |||
| labelHidden = true, | |||
| label, | |||
| name = 'q', | |||
There was a problem hiding this comment.
Why remove the default name? Without a name, the field value won't be submitted from within a form.
There was a problem hiding this comment.
ok, will restore in next commit
| @@ -68,13 +77,16 @@ export const SearchField: Primitive<SearchFieldProps, 'input'> = ({ | |||
| className, | |||
| labelHidden = true, | |||
| label, | |||
| name = 'q', | |||
| name, | |||
| role = 'searchbox', | |||
There was a problem hiding this comment.
I did some research on this and I don't think we should add this role here. I don't see role="searchbox" being used on any other search fields that I can find in the wild, and it actually has some drawbacks (see: w3c/aria-practices#1903)
From this comment it sounds like it would force the screen reader to read 'search' multiple times if the accessible name also has 'search' in it, which isn't very user friendly.
I think this is a case where applying "no ARIA is better than bad ARIA" rule of thumb makes sense.
There was a problem hiding this comment.
agree, fixed in next revision
| ListCollectionProps<CollectionType>; | ||
| export type CollectionProps<Item> = CollectionWrapperProps & | ||
| ( | ||
| | ({ type: 'list' } & ListCollectionProps<Item>) |
There was a problem hiding this comment.
Why move the { type: 'list' } down here?
There was a problem hiding this comment.
cause I don't want to expose the type property to ListCollection or GridCollection props.
Description of changes:
Introducing search & pagination capabilities for
<Collection>componentscollections.mov
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.