Create new GridList component based on old GridView #1914
Conversation
src/index.js
Outdated
| @@ -69,6 +69,9 @@ export default { | |||
| get GridListItem() { | |||
| return require('./components/gridListItem').default; | |||
| }, | |||
| get GridList() { | |||
| return require('./components/gridList').default; | |||
| }, | |||
| get GridView() { | |||
There was a problem hiding this comment.
Do we want to deprecate GridView?
There was a problem hiding this comment.
Yes, definitely, I'll create a separate ticket for that
src/components/gridList/index.tsx
Outdated
| /** | ||
| * whether to keep the items initial size when orientation changes, | ||
| * in which case the apt number of columns will be calculated automatically. | ||
| * Ignored when passing 'maxItemWidth' |
There was a problem hiding this comment.
Ignored when passing 'maxItemWidth' seems wrong
There was a problem hiding this comment.
Left overs from the old grid view implementation, removed
src/components/gridList/index.tsx
Outdated
| const containerWidth = useMemo(() => { | ||
| return (props.containerWidth ?? Constants.screenWidth) - listPadding * 2; | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [listPadding, orientation]); |
There was a problem hiding this comment.
Shouldn't props.containerWidth be in the dependencies (why should listPadding change and not containerWidth)?
There was a problem hiding this comment.
Added containerWidth (the comment still relevant because of the orientation dep)
Anyway, the code has been moved to useGridLayout hook
src/components/gridList/index.tsx
Outdated
|
|
||
| const _renderItem = useCallback((...args) => { | ||
| // @ts-expect-error | ||
| return <View style={[itemContainerStyle]}>{renderItem?.(...args)}</View>; |
src/components/gridList/index.tsx
Outdated
| /** | ||
| * List padding (used for item size calculation) | ||
| */ | ||
| listPadding?: number; |
There was a problem hiding this comment.
The listPadding is not acting as expected, for example change the screen to use:
numColumns={2}
// maxItemWidth={140}
itemSpacing={Spacings.s10}
listPadding={Spacings.s2}
There is no padding on the right side.
Or:
numColumns={2}
// maxItemWidth={140}
itemSpacing={Spacings.s10}
// listPadding={Spacings.s2}
It looks like the right item is a little out of the screen
There was a problem hiding this comment.
Well, technically the listPadding prop goes together with setting the list contentContainerStyle with padding.
But I see your point, I think I'll inject the padding in the list myself so the user won't have to worry about it
| <Text>{item.name}</Text> | ||
| <Text>{item.formattedPrice}</Text> | ||
| {item.inventory.status === 'Out of Stock' && ( | ||
| <Text text90M red30> |
There was a problem hiding this comment.
Consider using $textDangerLight
|
@M-i-k-e-l I fixed/comment everything, notice that I did a small refactor and moved the logic and calculation of the grid to useGridLayout hook (this is relevant to the next PR of the SortableGridList) |
ethanshar
added
the
Important for Next Release
src/components/gridList/index.tsx
Outdated
| const {itemContainerStyle, numberOfColumns} = useGridLayout({ | ||
| numColumns, | ||
| itemSpacing, | ||
| maxItemWidth, | ||
| listPadding, | ||
| keepItemSize | ||
| }); |
There was a problem hiding this comment.
You forgot to pass the containerWidth here
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [numColumns, maxItemWidth, itemSpacing, keepItemSize ? _containerWidth : undefined]); | ||
|
|
||
| const itemSize = useMemo(() => { |
There was a problem hiding this comment.
I think it's better to name it itemWidth since it does nothing with height.
| return {width: itemSize /* + itemSpacing */, marginRight: itemSpacing, marginBottom: itemSpacing}; | ||
| }, [itemSize, itemSpacing]); | ||
|
|
||
| return {itemContainerStyle, numberOfColumns, itemSize}; |
There was a problem hiding this comment.
I see that you return the itemSize but I don't see that we using it somewhere outside.
|
|
||
| renderItem: GridListProps<typeof products[0]>['renderItem'] = ({item}) => { | ||
| return ( | ||
| <Card flex> |
There was a problem hiding this comment.
Consider adding $textDefault for the texts inside the card
Description
Create new GridList component based on old GridView
Changelog
Create new GridList component based on old GridView features