Skip to content
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

Merged
merged 1 commit into from
Jun 11, 2019

Conversation

priley86
Copy link
Contributor

@priley86 priley86 commented Apr 22, 2019

Convert virtualized lists to @patternfly/react-tables and @patternfly/react-virtualization-extension.

tables-low-frame-min

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:
Screen Shot 2019-06-05 at 11 58 43 AM

Screen Shot 2019-06-05 at 12 03 29 PM

Screen Shot 2019-06-05 at 11 58 28 AM

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 new classNames transform to apply this.

  • Resolve CSS issues:

  • Ensure resizes are smooth and that we can flexibly define table breakpoint sizes for each column (i.e col-lg-3 col-md-4 col-sm-4) - todo: see if pf-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.
  • Fix text overflow issues in table cells. Codepen 2.

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

  • review header styles and table spacings w/ PF team
  • review Kebab styles / font sizes
  • add support for sortFunc
  • rebase latest patternfly release
  • error handling / empty state (use existing StatusBox)
  • custom id fields (support for custom guids/ids added, renders data-id attribute on td elements)
  • fix drag scroll with large data sets
  • fix slight overflow on md breakpoint due to sort icon (Alert Managers, Service Accounts, Role Bindings, Config Maps, Secrets), xl breakpoint (Horizontal Pod AutoScalers) (see priley86@0d106ea)
  • add StorageClassResourceKind
  • allow children of th cells to have different show/hide w/ pf-m-hidden and pf-m-visible-on-* (see StorageClassTableHeader) - resolved with pf-u-display-none-on-md pf-u-display-inline-block-on-lg utils
  • fix CI tests/kebab e2e tests
  • pull Kebab changes into separate Dropdown / Kebab PR
  • Leave out expand/collapse on Nodes table for now
  • implement accessible grid pattern for virtualized table
  • Refactor all List unit tests to use Table (subscriptions, package-manfiest, clusterserviceversion)
  • Style detail page headers and column headers
  • Start prototyping this in other OpenShift views. See if we can also look to add table selection support or other table features. Had prototyped this previously in Kubevirt Web UI

Resource Tables converted:

  • Projects
  • Search
  • Pods
  • Deployments
  • Deployment Config
  • Stateful Set
  • Secrets
  • Config Maps
  • Cron Jobs
  • Jobs
  • Daemon Sets
  • Replica Sets
  • Replication Controllers
  • Horizontal Pod Autoscalers
  • Services
  • Routes
  • Ingress
  • Network Policies
  • Persistent Volume
  • Persistent Volume Claim
  • Storage Class
  • Build Configs
  • Builds
  • Image Streams
  • Nodes
  • Machines
  • Machine Sets
  • Machine Deployments
  • Machine Config
  • Machine Config Pools
  • Machine Autoscalers
  • Namespaces
  • Service Accounts
  • Roles
  • Role Bindings
  • Resource Quotas
  • Limit Ranges
  • Custom Resource Definitions
  • Alert Manager (alertmanager.tsx)
    http://localhost:9000/k8s/all-namespaces/monitoring.coreos.com~v1~Alertmanager
  • Not visually tested: Chargeback (chargeback.tsx) (being revamped)
  • Not visually tested:Cluster Service Broker (cluster-service-broker.tsx)
  • Not visually tested:Cluster Service Class (cluster-service-class.tsx)
  • Not visually tested:Cluster Service Plan (cluster-service-plan.tsx)
  • Cluster Operators (cluster-operator.tsx)
    http://localhost:9000/settings/cluster/clusteroperators
  • Cluster Service Version (clusterserviceversion.tsx)
  • Not visually tested: Cluster Service Version Resource (clusterserviceversion-resource.tsx)
  • Operator Management - Operator Subscriptions (subscription.tsx)
  • Operator Management - Install Plan (install-plan.tsx)
  • Default Resource (default-resource.tsx)
  • Not visually tested:Monitoring / Alerts (monitoring.tsx)
  • Not visually tested:Prometheus (prometheus.jsx)
    http://localhost:9000/k8s/ns/openshift-monitoring/monitoring.coreos.com~v1~Prometheus
  • Not visually tested:Service Binding (service-binding.tsx)
  • Not visually tested:Service Instance (service-instance.tsx)
  • Not visually tested:Service Monitor (service-monitor.jsx)
    http://localhost:9000/k8s/ns/openshift-monitoring/monitoring.coreos.com~v1~ServiceMonitor
  • Not visually tested:Template Instance (template-instance.tsx)
  • remove existing List references
  • do not use export for tables, table rows, table headers, types, unless referenced

Detail Views/non-virtualized Tables converted (to be moved to separate PR):

  • Cluster Settings (cluster-settings.tsx) move into separate PR
  • k8s-resource.tsx
  • oauth.tsx
  • webhooks.tsx
  • container.tsx
  • chargeback.tsx
  • ingress.jsx
  • pod.tsx
  • package-manifest.tsx
  • container-table.tsx
  • endpoints.tsx
  • routes.tsx
  • volumes-table.tsx

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 22, 2019
frontend/package.json Outdated Show resolved Hide resolved
frontend/webpack.config.ts Outdated Show resolved Hide resolved
@spadgett spadgett changed the base branch from master to master-next April 22, 2019 15:11
Copy link
Member

@spadgett spadgett left a 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?

Deployments · OKD 2019-04-22 12-21-03

The kebab text is also now much larger than anything else, even the nav items. Is that intentional?

Deployments · OKD 2019-04-22 12-18-44

frontend/public/components/deployment.jsx Outdated Show resolved Hide resolved
frontend/public/components/deployment.jsx Outdated Show resolved Hide resolved
frontend/public/components/utils/kebab.tsx Outdated Show resolved Hide resolved
@priley86
Copy link
Contributor Author

re: header styles / margin spacings... these can all be easily adjusted. Will add to checklist above!

frontend/public/components/factory/table.tsx Outdated Show resolved Hide resolved
frontend/public/components/factory/table.tsx Outdated Show resolved Hide resolved
frontend/public/components/utils/kebab.tsx Outdated Show resolved Hide resolved
frontend/public/components/deployment.jsx Outdated Show resolved Hide resolved
@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 3, 2019
@priley86 priley86 force-pushed the react-tables-poc branch 2 times, most recently from 5ea6e7b to 5af50df Compare May 3, 2019 19:49
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 3, 2019
@priley86 priley86 force-pushed the react-tables-poc branch 3 times, most recently from a9a1dac to 482c381 Compare May 7, 2019 17:34
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 9, 2019
@priley86 priley86 force-pushed the react-tables-poc branch from 523afe4 to 48e0379 Compare May 9, 2019 21:19
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 9, 2019
@priley86 priley86 force-pushed the react-tables-poc branch from 48e0379 to f78cd02 Compare May 10, 2019 14:21
@spadgett
Copy link
Member

Great progress @priley86! This is a clear improvement. Some comments:

  • There's a lot of flickering and flashing when I resize my browser.
  • Initial table sort isn't indicated when loading the page.
  • The kebab being vertically centered instead of top aligned looks strange to me with taller rows. Rows with lots of labels can get pretty tall.
  • We should try to update all actions dropdowns if we're updating the kebab dropdown styles for consistency. The kebab in the list and the same actions on the details page have very different padding and styles. (Not necessarily in this PR.)

A couple general PF4 table comments:

  • The table headers look a lot like regular table text IMO. I feel like it would be better if there was some styling to visually distinguish them.
  • The two-headed arrow looks a little strange to me. I realize it's a sort indicator, but why does the arrow point both ways? :)

The good:

@spadgett spadgett changed the base branch from master-next to master May 13, 2019 22:44
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 14, 2019
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 11, 2019
@priley86
Copy link
Contributor Author

  • rebased

@priley86 priley86 marked this pull request as ready for review June 11, 2019 16:53
@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jun 11, 2019
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 11, 2019
@priley86
Copy link
Contributor Author

  • rebased chargeback.tsx

Copy link
Member

@spadgett spadgett left a 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/components/monitoring.tsx Outdated Show resolved Hide resolved
@import "~@patternfly/patternfly/_fonts";
@import "~@patternfly/patternfly/_base";
@import '~@patternfly/patternfly/layouts/Flex/flex';
Copy link
Member

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?

Copy link
Contributor Author

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/integration-tests/views/crud.view.ts Outdated Show resolved Hide resolved
@priley86
Copy link
Contributor Author

Thanks, Patrick. Looks good.

Is the plan to remove the existing List components in a follow on?

It looks like there is just a few remaining references. I am fine w/ waiting to convert the Metal3 and Kubevirt List's to Table's in a future PR since this PR is growing so large.

Copy link
Member

@spadgett spadgett left a 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

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 11, 2019
@openshift-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants