fix(datalist): actions wrapper #10939
Conversation
tlabaj
requested review from
mcoker,
a team,
wise-king-sullyman and
kmcfaul
and removed request for
a team
|
Preview: https://patternfly-react-pr-10939.surge.sh A11y report: https://patternfly-react-pr-10939-a11y.surge.sh |
| expect(screen.getByTestId('actions')).toHaveClass(styles.modifiers[`${visMod}OnMd`]); | ||
| expect(screen.getByTestId('actions')).toHaveClass(styles.modifiers[`${visMod}OnLg`]); | ||
| expect(screen.getByTestId('actions')).toHaveClass(styles.modifiers[`${visMod}OnXl`]); | ||
| expect(screen.getByTestId('actions')).toHaveClass(styles.modifiers[`${visMod}On_2xl`]); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Can you add a test which asserts that the children of the DataListAction now have the dataListAction class?
|
@andrew-ronaldson @lboehling just want to run this change by you and make sure this design change is OK. I created a codepen showing a regular data-list and one with the proposed change - https://codepen.io/mcoker/pen/PorgJab Just as a reminder, the TL;DR of this change is to update all of the "action" cells in a data-list to have a negative top/bottom margin that is equal to a button's top/bottom padding. We already do this for plain actions (kebabs in the codepen), and this will do the same for non-plain actions. What that effectively does is keeps the presence of a button in a row from increasing the height of the row, and it aligns the button text with the data-list text. On a row with a single line of text, this looks great IMO. It vertically centers the button in the row. Before After But when text wraps and the row is taller, it's more obvious that non-plain actions are shifted up. I just want to confirm that's ok from y'alls perspective: Before After |
There was a problem hiding this comment.
Discussed offline but looks like we may need to handle this a little differently. I think we'll probably just remove <div className={css(styles.dataListAction)}> and update core to apply the margin change to <div className={css(styles.dataListItemAction instead. So in this PR, we'd just remove that first div and the CSS will handle the rest.
But first I want to confirm with design that the proposed change is still 👍👍
|
@mcoker i'm good w this change! |
| ...props | ||
| }: DataListActionProps) => ( | ||
| <div className={css(styles.dataListItemAction, formatBreakpointMods(visibility, styles), className)} {...props}> | ||
| {isPlainButtonAction ? <div className={css(styles.dataListAction)}>{children}</div> : children} | ||
| <div className={css(styles.dataListAction)}>{children}</div> |
There was a problem hiding this comment.
This should be good to update now that patternfly/patternfly#7089 has been merged in core
What: Closes #10903
Additional issues: