-
Notifications
You must be signed in to change notification settings - Fork 667
patternfly react-tables #1465
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
patternfly react-tables #1465
Conversation
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 @priley86. Great to see progress on this. I'm glad that the changes to the individual pages aren't too bad.
I realize this is just a POC, but it looks like there is no longer any handling for errors or empty states.
It's too bad the table headers now have a different style than the dl elements on the details page. I liked that we had consistent header styles for resource properties on those two pages. This change makes things feel less unified.
Is there meant to be so many margin on either side of the table rows?
The kebab text is also now much larger than anything else, even the nav items. Is that intentional?
|
re: header styles / margin spacings... these can all be easily adjusted. Will add to checklist above! |
ae63e6e to
e2369ac
Compare
5ea6e7b to
5af50df
Compare
a9a1dac to
482c381
Compare
48e0379 to
f78cd02
Compare
|
Great progress @priley86! This is a clear improvement. Some comments:
A couple general PF4 table comments:
The good:
|
b25cfef to
09d43a6
Compare
|
f5839c6 to
2dabcec
Compare
|
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, Patrick. Looks good.
Is the plan to remove the existing List components in a follow on?
frontend/public/vendor.scss
Outdated
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.
Do we need these extra imports if we aren't using the flex utility classes?
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.
i've removed the flex utils and spacing utils. I am using one display util in storage-class.tsx here:
pf-u-display-none-on-md pf-u-display-inline-block-on-lg
these display helpers are already responsive and MQ safe. If you want to remove them we can though, but I find them quite comparable to Bootstrap's hidden-[breakpoint] classes.
frontend/public/components/operator-lifecycle-manager/_operator-lifecycle-manager.scss
Outdated
Show resolved
Hide resolved
2dabcec to
64b40ef
Compare
It looks like there is just a few remaining references. I am fine w/ waiting to convert the Metal3 and Kubevirt |
64b40ef to
058d3dd
Compare
72e0ee6 to
0324863
Compare
spadgett
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.
Looks good! Thanks @priley86
/lgtm
/approve
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: priley86, spadgett The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |


Convert virtualized lists to @patternfly/react-tables and @patternfly/react-virtualization-extension.
Associated @patternfly/react-virtualized-extension PR:
patternfly/patternfly-react#2011
Demo of virtualized tables:
https://2011-pr-patternfly-react-patternfly.surge.sh/patternfly-4/virtual%20scroll/virtualized/
Update Column Headers/Detail Header styles:

Changes:
Ensure CSS will support hiding/showing table columns on mobile the same way Bootstrap 3 grid (i.e.
hidden-xs) does today here. Support recently added in PF Core and PF React. Will use the newclassNamestransform to apply this.Resolve CSS issues:
col-lg-3 col-md-4 col-sm-4) - todo: see ifpf-m-width-[10, 15, 20, 25, 30, 35, 40, 45, 50, 60, 70, 80, or 90],pf-m-width-max,pf-m-fit-content(from pf4 table docs) will support or if additional CSS is needed. Update: PF Core reviewing custom utilities and downsream css we can add for this Codepen 1.Corrected in Codepen 3 (minus the Firefox/Edge word-break mixin applied in OpenShift w/
co-break-word)Additional codepen samples:
Converted classes - Virtualized Pods Markup
Details View - Non-Virtualized Markup
sortFuncdata-idattribute ontdelements)StorageClassResourceKindthcells to have different show/hide w/pf-m-hiddenandpf-m-visible-on-*(see StorageClassTableHeader) - resolved withpf-u-display-none-on-md pf-u-display-inline-block-on-lgutilsListunit tests to useTable(subscriptions, package-manfiest, clusterserviceversion)Resource Tables converted:
http://localhost:9000/k8s/all-namespaces/monitoring.coreos.com~v1~Alertmanager
http://localhost:9000/settings/cluster/clusteroperators
http://localhost:9000/k8s/ns/openshift-monitoring/monitoring.coreos.com~v1~Prometheus
http://localhost:9000/k8s/ns/openshift-monitoring/monitoring.coreos.com~v1~ServiceMonitor
exportfor tables, table rows, table headers, types, unless referencedDetail Views/non-virtualized Tables converted (to be moved to separate PR):
Cluster Settings (cluster-settings.tsx)move into separate PR