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

New virtualized table component #8829

Merged
merged 3 commits into from Aug 25, 2021

Conversation

rawagner
Copy link
Contributor

@rawagner rawagner commented Apr 30, 2021

This PR revamps table component. Depends on revamped list pages #8827 . See only last commit. Converts PodList and MachineList pages.

New table is virtualized only. Non-virtualized table should be a separate component to keep the amount of props to minimum IMO.

Changes in new table

  1. filtering - sice new list pages are taking care of filtering New ListPage components #8827 there is no need to have the functionality in the new table which simplified things quite a bit
  2. sorting - colum (previously called Header, i've changed that to align with PF) provides a sort function - a specific implementation not just some string which loosly references implementation (previously sortFunc). sort prop also accepts string selector.

Prevous header/sorting definition

const getHeader = (showNodes) => [
    {
      title: i18next.t(podColumnInfo.name.title),
      id: podColumnInfo.name.id,
      sortField: 'metadata.name', // or sortFunc: 'fooFunc'
      transforms: [sortable],
      props: { className: podColumnInfo.name.classes },
    },
];

new way - almost the same, but we are in control when sorting

const getColumns = (showNodes: boolean, t: TFunction): TableColumn<PodKind>[] => [
  {
    title: t(podColumnInfo.name.title),
    id: podColumnInfo.name.id,
    sort: (data, direction) => { // data and direction is properly typed as PodKind[] and SortByDirection
      // do whatever sorting you need
      return data.sort(sortResourceByValue<PodKind>(direction, podRestarts));
    },
    // or a simple string selector
    sort: 'metadata.name'
    transforms: [sortable],
    props: { className: podColumnInfo.name.classes },
  },
];

memoize the colums (headers) and pass them to new VirtualizedTable

// ....
const columns = React.useMemo(() => getColumns(showNodes, t), [showNodes, t]);
return (
  <VirtualizedTable<PodKind>
    data={data}
    loaded={loaded}
    loadError={loadError}
    aria-label={t('public~Pods')}
    columns={columns}
    Row={Row}
  />
);
  1. Row prop is evaluated as an actual React.FC not a function which returns a JSX.
  2. no more customData prop drilling
    Wrap your Row component into useCallback and pass whatever data you want.
const Row = React.useCallback<React.FC<RowProps<PodKind>>>(
  (rowProps) => (
    <PodTableRow
      {...rowProps}
      // these are the old `customData`
      showNamespaceOverride={showNamespaceOverride}
      showNodes={showNodes}
    />
  ),
  [showNamespaceOverride, showNodes],
);

as @TheRealJon mentioned in some of my other PR, doing this is antipattern so I reverted this change. You can still pass additional data to rows by rowData prop. rowData is more descriptive name than customData IMO.

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rawagner

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

@openshift-ci-robot openshift-ci-robot added the component/core Related to console core functionality label Apr 30, 2021
@openshift-ci-robot openshift-ci-robot added component/helm Related to helm-plugin approved Indicates a PR has been approved by an approver from all required OWNERS files. component/kubevirt Related to kubevirt-plugin component/lso Related to local-storage-operator-plugin component/olm Related to OLM component/pipelines Related to pipelines-plugin component/shared Related to console-shared kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated labels Apr 30, 2021
@rawagner rawagner force-pushed the table_new branch 4 times, most recently from 7cdbf54 to d7bc64a Compare April 30, 2021 12:01
@spadgett
Copy link
Member

This is great to see and looks like a much cleaner API. Thank you @rawagner 👍

I did want to mention we have a long standing bug with table performance tracked in https://bugzilla.redhat.com/show_bug.cgi?id=1856355 and patternfly/react-virtualized-extension#8. @jcaianirh and @dtaylor113 have been looking at it. If we're going to be reworking our tables in console anyway, it might be a good time to try to tackle that, particularly if it requires changes that touch all pages. @jcaianirh mentioned that the PatternFly composable table might perform better, but I'm not sure if that works with virtualization or how big a change that is. It's possible the performance issue is just an upstream PatternFly bug, but I wanted to raise it.

Ideally virtualization would just be a single boolean prop to enable or disable, but I'm not sure if that's possible since the PatternFly virtualized and non-virtualized tables have different APIs.

@rawagner
Copy link
Contributor Author

rawagner commented May 3, 2021

I tried composable table (example with virtualized table is here ) but I didnt see any performance improvement on pods page :/

@rawagner rawagner force-pushed the table_new branch 2 times, most recently from e5d76d2 to cae0c76 Compare May 4, 2021 14:10
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 4, 2021
@rawagner rawagner force-pushed the table_new branch 2 times, most recently from 56ff9f1 to 3cd3ef5 Compare May 4, 2021 16:51
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 4, 2021
@rawagner rawagner force-pushed the table_new branch 4 times, most recently from 29f2299 to 7bcb701 Compare May 5, 2021 10:29
@rawagner
Copy link
Contributor Author

rawagner commented May 5, 2021

@spadgett i've made some changes to cell cache and wrapped row rendering functions in useCallback and I think there's is a difference when scrolling but it still isnt fluid.

@openshift-ci openshift-ci bot added px-approved Signifies that Product Support has signed off on this PR docs-approved Signifies that Docs has signed off on this PR labels Aug 19, 2021
@rawagner rawagner force-pushed the table_new branch 2 times, most recently from 284f2ff to 568f301 Compare August 19, 2021 13:30
@rawagner
Copy link
Contributor Author

rawagner commented Aug 19, 2021

Ok, I added one last change - useActiveColumns (see latest commit) which loads user settings & filters the table columns based on those settings. The filtered columns are then passed to the table. The table is handling only column visibility based on the actual size.

The table how has 3 less properties and IMO its more transparent to see whats going on.

@rawagner rawagner force-pushed the table_new branch 6 times, most recently from 6473919 to e10dd52 Compare August 20, 2021 07:44
@rawagner
Copy link
Contributor Author

/retest

@rawagner
Copy link
Contributor Author

/retest

@yapei
Copy link
Contributor

yapei commented Aug 24, 2021

Checked sorting, filtering, column management and responsive of the table on Pods and Machines list page, looks good
/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Aug 24, 2021
@rawagner
Copy link
Contributor Author

/retest

frontend/public/components/cron-job.tsx Show resolved Hide resolved
frontend/public/components/pod.tsx Show resolved Hide resolved
frontend/public/components/pod.tsx Show resolved Hide resolved
frontend/public/components/pod.tsx Show resolved Hide resolved
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.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 24, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 24, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rawagner, 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

@rawagner
Copy link
Contributor Author

/retest

1 similar comment
@rawagner
Copy link
Contributor Author

/retest

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. component/ceph Related to ceph-storage-plugin component/core Related to console core functionality component/helm Related to helm-plugin component/knative Related to knative-plugin component/kubevirt Related to kubevirt-plugin component/lso Related to local-storage-operator-plugin component/metal3 Related to metal3-plugin component/monitoring Related to monitoring component/olm Related to OLM component/pipelines Related to pipelines-plugin component/shared Related to console-shared docs-approved Signifies that Docs has signed off on this PR kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants