-
Notifications
You must be signed in to change notification settings - Fork 618
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
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
@import "~@patternfly/patternfly/_fonts"; | ||
@import "~@patternfly/patternfly/_base"; | ||
@import '~@patternfly/patternfly/layouts/Flex/flex'; |
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
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
Needs 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 newclassNames
transform 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
sortFunc
data-id
attribute ontd
elements)StorageClassResourceKind
th
cells to have different show/hide w/pf-m-hidden
andpf-m-visible-on-*
(see StorageClassTableHeader) - resolved withpf-u-display-none-on-md pf-u-display-inline-block-on-lg
utilsList
unit 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
export
for 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