-
Notifications
You must be signed in to change notification settings - Fork 316
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
<Collection> improvements: isPagination/isSearchable #507
Changes from 6 commits
b722443
c9850dc
ab5bca1
5560f86
603e1e3
b8413e7
e2e514b
f2a6d0c
e01b957
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,18 +1,101 @@ | ||
import * as React from 'react'; | ||
import { CollectionProps } from '../types'; | ||
import debounce from 'lodash/debounce'; | ||
import { useCallback, useState } from 'react'; | ||
import { Flex } from '../Flex'; | ||
import { Grid } from '../Grid'; | ||
import { Pagination, usePagination } from '../Pagination'; | ||
import { SearchField } from '../SearchField'; | ||
import { strHasLength } from '../shared/utils'; | ||
import { | ||
CollectionProps, | ||
GridCollectionProps, | ||
ListCollectionProps, | ||
} from '../types'; | ||
import { getItemsAtPage, itemHasText } from './utils'; | ||
|
||
export const Collection = <CollectionItemType,>({ | ||
const DEFAULT_PAGE_SIZE = 10; | ||
const TYPEAHEAD_DELAY_MS = 300; | ||
|
||
const ListCollection = <Item,>({ | ||
children, | ||
items, | ||
...rest | ||
}: ListCollectionProps<Item>) => ( | ||
<Flex {...rest}>{Array.isArray(items) && items.map(children)}</Flex> | ||
); | ||
|
||
const GridCollection = <Item,>({ | ||
children, | ||
className, | ||
direction = 'column', | ||
items, | ||
...rest | ||
}: GridCollectionProps<Item>) => ( | ||
<Grid {...rest}>{Array.isArray(items) && items.map(children)}</Grid> | ||
); | ||
|
||
export const Collection = <Item,>({ | ||
isSearchable, | ||
isPaginated, | ||
items, | ||
itemsPerPage = DEFAULT_PAGE_SIZE, | ||
searchFilter = itemHasText, | ||
searchPlaceholder, | ||
type = 'list', | ||
...rest | ||
}: CollectionProps<CollectionItemType>): JSX.Element => { | ||
}: CollectionProps<Item>): JSX.Element => { | ||
const [searchText, setSearchText] = useState<string>(); | ||
|
||
const onSearch = useCallback(debounce(setSearchText, TYPEAHEAD_DELAY_MS), [ | ||
setSearchText, | ||
]); | ||
|
||
// Make sure that items are iterable | ||
items = Array.isArray(items) ? items : []; | ||
|
||
// Filter items by text | ||
if (isSearchable && strHasLength(searchText)) { | ||
items = items.filter((item) => searchFilter(item, searchText)); | ||
} | ||
|
||
// Pagination | ||
const pagination = usePagination({ | ||
totalPages: Math.floor(items.length / itemsPerPage), | ||
}); | ||
|
||
if (isPaginated) { | ||
items = getItemsAtPage(items, pagination.currentPage, itemsPerPage); | ||
} | ||
|
||
const collection = | ||
type === 'list' ? ( | ||
<ListCollection items={items} {...rest} /> | ||
) : type === 'grid' ? ( | ||
<GridCollection items={items} {...rest} /> | ||
) : null; | ||
|
||
if (!isSearchable && !isPaginated) { | ||
return collection; | ||
} | ||
|
||
return ( | ||
<Flex direction={direction} className={className} {...rest}> | ||
{Array.isArray(items) && items.map(children)} | ||
<Flex direction="column"> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good idea, will add in next commit |
||
{isSearchable && ( | ||
<Flex direction="row" justifyContent="center"> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment as above on moving style props styling to CSS file. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done in next revision |
||
<SearchField | ||
size="small" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be customizable by the user? What if the customer wants a different size or variation? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed in next revision |
||
label="Search" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done in next revision |
||
placeholder={searchPlaceholder} | ||
onChange={(e) => onSearch(e.target.value)} | ||
onClear={() => setSearchText('')} | ||
/> | ||
</Flex> | ||
)} | ||
|
||
{collection} | ||
|
||
{isPaginated && ( | ||
<Flex justifyContent="center"> | ||
<Pagination {...pagination} /> | ||
</Flex> | ||
)} | ||
</Flex> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ import { kebabCase } from 'lodash'; | |
|
||
import { Collection } from '../Collection'; | ||
import { ComponentPropsToStylePropsMap } from '../../types'; | ||
import { ComponentClassNames } from '../../shared/constants'; | ||
|
||
const emojis = [ | ||
{ | ||
|
@@ -22,9 +23,52 @@ const emojis = [ | |
describe('Collection component', () => { | ||
const testList = 'testList'; | ||
|
||
it('should render a Search box when isSearchable is true', async () => { | ||
render( | ||
<Collection testId={testList} type="list" items={emojis} isSearchable> | ||
{(item, index) => ( | ||
<div key={index} aria-label={item.title}> | ||
{item.emoji} | ||
</div> | ||
)} | ||
</Collection> | ||
); | ||
|
||
const searchField = await screen.findByRole('searchbox'); | ||
expect(searchField).not.toBe(undefined); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you check what findByRole returns if field is not found? I seem to remember it returning null rather than undefined when not found. |
||
}); | ||
|
||
it('should render pagination when isPaginated is true', async () => { | ||
render( | ||
<Collection | ||
testId={testList} | ||
type="list" | ||
items={emojis} | ||
isPaginated | ||
itemsPerPage={1} | ||
> | ||
{(item, index) => ( | ||
<div key={index} aria-label={item.title}> | ||
{item.emoji} | ||
</div> | ||
)} | ||
</Collection> | ||
); | ||
|
||
const navigation = await screen.findByRole('navigation'); | ||
|
||
expect(navigation.classList).toContain(ComponentClassNames.Pagination); | ||
expect(navigation).not.toBe(undefined); | ||
}); | ||
|
||
it('should render Flex when rendering list collection', async () => { | ||
render( | ||
<Collection testId={testList} type="list" items={emojis}> | ||
<Collection | ||
testId={testList} | ||
type="list" | ||
direction="column" | ||
items={emojis} | ||
> | ||
{(item, index) => ( | ||
<div key={index} aria-label={item.title}> | ||
{item.emoji} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
import { strHasLength } from '../shared/utils'; | ||
|
||
/** | ||
* Slice a collection based on page index (starting at 1) | ||
*/ | ||
export const getItemsAtPage = <T>( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add unit tests for these? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added in next version |
||
items: T[], | ||
page: number, | ||
itemsPerPage: number | ||
) => { | ||
const startIndex = (page - 1) * itemsPerPage; | ||
return items.slice(startIndex, startIndex + itemsPerPage); | ||
}; | ||
|
||
/** | ||
* Recursively find a keyword within an object (case insensitive) | ||
*/ | ||
export const itemHasText = (item: unknown, text: string): boolean => { | ||
if (strHasLength(item)) { | ||
return item.toLowerCase().includes(text.toLowerCase()); | ||
} | ||
|
||
if (typeof item === 'object') { | ||
return Object.values(item).some((subItem) => itemHasText(subItem, text)); | ||
} | ||
|
||
return false; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we explain what the default search function is, or do they have to provide a search function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed