-
Notifications
You must be signed in to change notification settings - Fork 357
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(datalist): actions wrapper #10939
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be good to update now that patternfly/patternfly#7089 has been merged in core
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LPTM! 🎸🤘
What: Closes #10903
Additional issues: