Conversation
|
Preview: https://patternfly-react-pr-10251.surge.sh A11y report: https://patternfly-react-pr-10251-a11y.surge.sh |
tlabaj
requested review from
a team,
kmcfaul,
mattnolting,
mmenestr,
srambach and
tlabaj
and removed request for
a team and
mattnolting
| aria-labelledby="single-action-item1 single-action-action1" | ||
| id="single-action-action1" | ||
| aria-label="Actions" |
There was a problem hiding this comment.
These 3 props can be removed from DataListAction here and the other instance of the component. They're not currently being applied anywhere internally.
Ideally they could be applied to the Button (or whatever interactive action) that is passed as children. Depending on whether this demo should be more 1:1 to the Core demo (where each DataListItem as the same "Delete" and "Link" action visually), then we could move the aria-labelledby and id props to those components.
There was a problem hiding this comment.
These 3 props can be removed from DataListAction here and the other instance of the component. They're not currently being applied anywhere internally.
These 3 props are mandatory fields , so we can't remove
Ideally they could be applied to the Button (or whatever interactive action) that is passed as children. Depending on whether this demo should be more 1:1 to the Core demo (where each DataListItem as the same "Delete" and "Link" action visually), then we could move the aria-labelledby and id props to those components.
There was a problem hiding this comment.
@tlabaj would there be any objection to either marking these props as optional or removing them from the DataListAction interface? We'd still be destructuring them where are are currently so they wouldn't actually be applied to anything, so it shouldn't really cause any issues. It would just prevent us from having to pass in props that aren't doing anything like this.
There was a problem hiding this comment.
We do have an issue tracking this #9823 it just needs to be prioritized.
packages/react-core/src/demos/DataList/examples/DataListActions.tsx
Outdated
Show resolved
Hide resolved
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
Above comment isn't a blocker re: the props that aren't actually doing anything. One comment below, and like other demo PRs I imagine we may want to have this demo match more closely to how Core's actionable example is setup but should wait for input from design cc @mmenestr
packages/react-core/src/demos/DataList/examples/DataListActions.tsx
Outdated
Show resolved
Hide resolved
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
Other than input on the contents of the demo compared to the Core demo, lgtm
|
This PR has been automatically marked as stale because it has not had recent activity. |
|
This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
Will be closing in favor of #11402 to target our main v6 branch. |
6 participants
What: Closes #10243
Add DataList actions demo
Additional issues: