-
Notifications
You must be signed in to change notification settings - Fork 377
feat(Table): Add columnTransforms, classNames decorator, Visibility constants, and hidden/visible breakpoints example #1858
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
feat(Table): Add columnTransforms, classNames decorator, Visibility constants, and hidden/visible breakpoints example #1858
Conversation
|
looking good! |
78fe6a1 to
2cd439f
Compare
|
PatternFly-React preview: https://1858-pr-patternfly-react-patternfly.surge.sh |
…onstants, and hidden/visible breakpoints example Closes patternfly#1832
2cd439f to
e2b80fa
Compare
Codecov Report
@@ Coverage Diff @@
## master #1858 +/- ##
=========================================
Coverage ? 82.59%
=========================================
Files ? 622
Lines ? 6849
Branches ? 93
=========================================
Hits ? 5657
Misses ? 1152
Partials ? 40
Continue to review full report at Codecov.
|
redallen
left a comment
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 confirm it works!
| import styles from '@patternfly/patternfly/components/Table/table.css'; | ||
|
|
||
| const pickProperties = (object, properties) => | ||
| properties.reduce((picked, property) => ({ ...picked, [property]: object[property] }), {}); |
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.
Probably a better way to do (and standardize) this across more components than just table, but this works for now.
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.
Yeah I agree, I don't think there is a clean way to get a subset of an object's properties. I figured this was nicer than the repetition in my original version:
const {
hidden,
hiddenOnSm,
hiddenOnMd,
hiddenOnLg,
hiddenOnXl,
visibleOnSm,
visibleOnMd,
visibleOnLg,
visibleOnXl
} = styles.modifiers;
export const Visibility = {
hidden,
hiddenOnSm,
hiddenOnMd,
hiddenOnLg,
hiddenOnXl,
visibleOnSm,
visibleOnMd,
visibleOnLg,
visibleOnXl
};
| columns: [ | ||
| { | ||
| title: 'Repositories', | ||
| columnTransforms: [classNames(Visibility.hidden, Visibility.visibleOnMd, Visibility.hiddenOnLg)] |
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.
Love this interface.
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.
Thanks, me too :)
mcoker
left a comment
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.
hidden/visible functionality looks great!!
tlabaj
left a comment
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
What: Closes #1832
This PR adds:
columnTransformswhich can be included in a column definition (object in thecellsarray of aTable) with an array of reactabular transform functions which will apply to every cell in the column (header and body). This complements the existing propertiestransforms(only applies to header cells) andcellTransforms(only applies to body cells).classNameswhich just applies all of its arguments as classNames to the affected cells.Visibilitywhich includes all the modifiers for column visibility as properties.