refactor(DataList): Refactor DataList based on changes in core #1755
Conversation
|
PatternFly-React preview: https://1755-pr-patternfly-react-patternfly.surge.sh |
Codecov Report
@@ Coverage Diff @@
## master #1755 +/- ##
==========================================
+ Coverage 82.72% 82.77% +0.04%
==========================================
Files 598 601 +3
Lines 6629 6642 +13
Branches 72 72
==========================================
+ Hits 5484 5498 +14
+ Misses 1118 1117 -1
Partials 27 27
Continue to review full report at Codecov.
|
|
PatternFly-React preview: https://1755-pr-patternfly-react-patternfly.surge.sh |
| @@ -1,10 +1,11 @@ | |||
| import { FunctionComponent, HTMLProps, ReactNode } from 'react'; | |||
|
|
|||
| export interface DataListActionProps extends HTMLProps<HTMLDivElement> { | |||
| children: any; | |||
| 'aria-labelledby': string; | ||
| 'aria-label': string; | ||
| id: string; | ||
| actions: ReactNode[]; | ||
| actions?: ReactNode[]; |
| disabled={isDisabled} | ||
| checked={isChecked || checked} | ||
| /> | ||
| <div className={css(styles.dataListItemControl, className)} {...props}> |
There was a problem hiding this comment.
props are spread twice here and in input
| @@ -1,7 +1,8 @@ | |||
| export { default as DataList } from './DataList'; | |||
| export { default as DataListItem } from './DataListItem'; | |||
| export { default as DataListAction } from './DataListAction'; | |||
There was a problem hiding this comment.
We usually also export out the *Props, e.g. DataListActionProps
There was a problem hiding this comment.
So add it for all of t hem correct? even the ones that were not updated?
| /** Additional classes added to the DataList item Content Wrapper. Children should be one ore more <DataListCell> nodes */ | ||
| className: PropTypes.string, | ||
| /** Array of <DataListCell> nodes that are rendered one after the other. */ | ||
| dataListCells: PropTypes.arrayOf(PropTypes.node).isRequired, |
There was a problem hiding this comment.
We already compose
<DataList>
<DataListItem>
<DataListItemCells />
</DataListItem>
</DataList>
Then why not also <DataListCell> into DataListItemCells?
There was a problem hiding this comment.
I want to enforce the consumer using DataListItemsCell wrapper around the DataListItems. That is why I have the prop. Does that answer you question? I am not sure I understand it.
There was a problem hiding this comment.
Consumer could still pass anything they want into the dataListCells prop... why not
<DataListItemCells>
<DataListCell>...</DataListCell>
</DataListItemCells>
|
Also looks like you included the third data list item within the second data list, thats why they're appearing side by side like this:
|
#1747
What:
Additional issues: