-
Notifications
You must be signed in to change notification settings - Fork 364
fix(Table): removed divider for expanded rows #11815
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
base: main
Are you sure you want to change the base?
fix(Table): removed divider for expanded rows #11815
Conversation
@@ -29,8 +29,8 @@ | |||
"clean": "rimraf dist" | |||
}, | |||
"devDependencies": { | |||
"@patternfly/patternfly": "6.3.0-prerelease.6", | |||
"css": "^3.0.0", | |||
"@adobe/css-tools": "^4.4.2", |
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.
Context: the original css package was last updated in 2020 and we were getting build errors due to the @starting-style
property from Core. This new package is a fork of the original but more up to date.
Preview: https://patternfly-react-pr-11815.surge.sh A11y report: https://patternfly-react-pr-11815-a11y.surge.sh |
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.
Design approved
@@ -96,7 +99,7 @@ const TrBase: React.FunctionComponent<TrProps> = ({ | |||
className={css( | |||
styles.tableTr, | |||
className, | |||
isExpanded !== undefined && styles.tableExpandableRow, | |||
isExpandable !== undefined && styles.tableExpandableRow, |
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.
Does this break people who were relying on the isExpanded
prop do apply the expandable row styles?
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.
isExpanded
still controls the expanded
modifier so if anyone is detecting expansion based on a class presence, I think they should be testing for pf-m-expanded
.
The issue is that the current block of logic isn't applying tableExpandableRow
when a row is collapsed (because isExpanded = false
is falsy), and it needs the class to be present on any row that can expand regardless of the expanded state. After thinking about it more, I think it might be possible to update the logic here to (isExpanded == true || isExpanded == false) && styles.tableExpandableRow
though, to avoid needing the new modifier?
What: Closes #11812, closes #11807, closes #11808
Table was updated to a more correct logic of applying certain classes from Core as were a few examples updates to show the correct way to set the tables up with those closes.
The animations aspect just needs to have confirmed that accordion, expandable section, and table expansion look good.
Additional issues: