feat(DataList): Add simple data list#927
feat(DataList): Add simple data list#927jschuler merged 1 commit intopatternfly:masterfrom boaz0:datalist
Conversation
|
PatternFly-React preview: https://927-pr-patternfly-react-patternfly.surge.sh |
Pull Request Test Coverage Report for Build 3448
💛 - Coveralls |
|
Hi @boaz1337! I thought I would follow up here, and ask for an update. Where are we at on this issue right now? |
|
Hi @rachael-phillips |
|
Thank you for the update, @boaz1337 ! |
boaz0
changed the title
| import React from 'react'; | ||
| import { css } from '@patternfly/react-styles'; | ||
| import PropTypes from 'prop-types'; | ||
| import styles from '@patternfly/patternfly-next/components/DataList/styles.css'; |
There was a problem hiding this comment.
Not an issue in your code, but i think this css should have a more specific name, will address with Core
There was a problem hiding this comment.
|
|
||
| const DataList = ({ children, className, 'aria-label': ariaLabel, ...props }) => { | ||
| return ( | ||
| <ul className={css(styles.dataList, boxShStyles.boxShadowMd, className)} role="list" aria-label={ariaLabel} {...props}> |
There was a problem hiding this comment.
Ideally this box shadow class is rolled into the data list class so we wouldn't have to import it. Also addressing with Core
There was a problem hiding this comment.
packages/patternfly-4/react-core/src/components/DataList/DataListItem.d.ts
Outdated
Show resolved
Hide resolved
packages/patternfly-4/react-core/src/components/DataList/examples/ExpandableDataList.js
Outdated
Show resolved
Hide resolved
packages/patternfly-4/react-core/src/components/DataList/examples/ExpandableDataList.js
Show resolved
Hide resolved
|
|
||
| const DataListAction = ({ className, ...props }) => ( | ||
| <div className={css(styles.dataListAction, className)} {...props}> | ||
| <Button variant="plain" aria-label="Actions"> |
There was a problem hiding this comment.
I think we want to get this through a prop, can default to Actions perhaps
There was a problem hiding this comment.
Also needs aria-labelledby
aria-labelledby="{title_cell_id} {data_list_action_id}"
See:
https://pf-next.com/components/DataList/examples/
There was a problem hiding this comment.
No problem but just wanted to hear your opinion.
It seems like Dropdown is more suitable here than Button?
What do you think?
There was a problem hiding this comment.
If we use the Dropdown in the example, I assume then it would show a menu on click? If that's the case, then I think a Dropdown would be better.
Also, does this implementation allow the consumer to use a single action button if that's what their design calls for?
There was a problem hiding this comment.
@jgiardino - yeah that's what I thought. I will change the implementation to use Dropdown.
Also, I will work on the case when there is a single action.
Thanks for your help!
packages/patternfly-4/react-core/src/components/DataList/DataListCell.js
Show resolved
Hide resolved
packages/patternfly-4/react-core/src/components/DataList/DataListCell.d.ts
Show resolved
Hide resolved
packages/patternfly-4/react-core/src/components/DataList/examples/ExpandableDataList.js
Outdated
Show resolved
Hide resolved
|
I just reviewed this with JAWS, and the controls are announced as expected. Great job on this! I expected all the combinations of |
christiemolloy
left a comment
christiemolloy
left a comment
There was a problem hiding this comment.
This looks spot on to the example we have in Core !
In Data List with modifiers, example 3, can you make that expandable functionality work?
Signed-off-by: Boaz Shuster <boaz.shuster.github@gmail.com>
|
I updated the examples as you suggested. Thanks. |
|
@boaz1337 this looks awesome! thanks for updating that |
9 participants
This is still work in progress. Hopefully it will be ready by next week.