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

Add support for custom table filters #1735

Merged

Conversation

vojtechszocs
Copy link
Contributor

@vojtechszocs vojtechszocs commented Jun 17, 2019

Table and List components now support custom filter functions via existing ListPage.rowFilters prop pass-through.

For example:

// packages/kubevirt-plugin/src/components/table-filters.ts

import { TableFilter } from '@console/internal/components/factory/table-filters';

export const kubevirtTableFilters: TableFilters = {
  VmStatus: {
    type: 'vm-status',
    filter: (statuses, vm) => {
      const status = getSimpleVmStatus(vm);
      return statuses.selected.has(status) || !_.includes(statuses.all, status);
    },
  },
};

type TableFilters = {
  [name: string]: {
    type: string;
    filter: TableFilter;
  };
};
// packages/kubevirt-plugin/src/components/vm.tsx

import { kubevirtTableFilters } from './table-filters';

const filters = [{
  type: kubevirtTableFilters.VmStatus.type, // change here
  selected: VM_SIMPLE_STATUS_ALL,
  reducer: getSimpleVmStatus,
  items: VM_SIMPLE_STATUS_ALL.map((status) => ({
    id: status,
    title: VM_SIMPLE_STATUS_TO_TEXT[status],
  })),
  filter: kubevirtTableFilters.VmStatus.filter, // change here
}];

// declare VirtualMachinesPage as usual, passing rowFilters={filters}

Additionally, Table vs. List filter map inconsistency was removed by using the same tableFilters implementation.


/cc @spadgett @christianvogt @mareklibra @jtomasek

@openshift-ci-robot
Copy link
Contributor

@vojtechszocs: GitHub didn't allow me to request PR reviews from the following users: mareklibra.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

This PR makes Table and List components support custom filter functions via customFilterFuncs prop, for example:

const kubevirtFilterFuncs = {
 'vm-status': (statuses, vm) => {
   const status = getSimpleVmStatus(vm);
   return statuses.selected.has(status) || !_.includes(statuses.all, status);
 },
};

const VirtualMachinesPage = (props) => (
 <ListPage
   {...props}
   canCreate
   kind={VirtualMachineModel.kind}
   ListComponent={VMList}
   createProps={getCreateProps(props.namespace)}
   rowFilters={filters}
   customFilterFuncs={kubevirtFilterFuncs}
 />
);

Built-in tableFilters take precedence over customFilterFuncs:

const filter = tableFilters[name] || customFilterFuncs[name];

Additionally, Table vs. List built-in filter inconsistency was removed by using the same tableFilters implementation.


/cc @spadgett @christianvogt @mareklibra @jtomasek

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 17, 2019
This was referenced Jun 17, 2019
@vojtechszocs
Copy link
Contributor Author

FYI @jelkosz

@vojtechszocs
Copy link
Contributor Author

@spadgett @alecmerdler Can you please share your knowledge on rowFilters prop of ListPage component?

For example, in public/components/pod.tsx

const filters = [{
  type: 'pod-status',
  selected: [ 'Running', 'Pending', 'Terminating', 'CrashLoopBackOff' ],
  reducer: podPhaseFilterReducer,
  items: [
    { id: 'Running', title: 'Running' },
    { id: 'Pending', title: 'Pending' },
    { id: 'Terminating', title: 'Terminating' },
    { id: 'CrashLoopBackOff', title: 'CrashLoopBackOff' },
    // Use title "Completed" to match what appears in the status column for the pod.
    // The pod phase is "Succeeded," but the container state is "Completed."
    { id: 'Succeeded', title: 'Completed' },
    { id: 'Failed', title: 'Failed' },
    { id: 'Unknown', title: 'Unknown '},
  ],
}];

The rowFilters prop is used to construct RowsOfRowFilters in ListPageWrapper_ representing the various (visual) filter components, and is therefore unrelated to what we're trying to do in this PR, right?

@christianvogt
Copy link
Contributor

christianvogt commented Jun 17, 2019

As discussed, I do not understand why rowFilters is disconnected from the filter function declarations which are currently mapped by type string inside table.tsx and list.tsx. Why the decoupling?
Could we not add a filter: (selected, all) => boolean property to the filter definition objects?

@spadgett
Copy link
Member

As discussed, I do not understand why rowFilters is disconnected from the filter function declarations which are currently mapped by type string inside table.tsx and list.tsx. Why the decoupling?
Could we not add a filter: (selected, all) => boolean property to the filter definition objects?

I agree, that's a cleaner approach. I don't know the history of the current implementation. @alecmerdler might.

We should consider the same thing for sortFunc, which follows the same pattern.

@spadgett spadgett added this to the v4.2 milestone Jun 17, 2019
@vojtechszocs
Copy link
Contributor Author

PR updated, added KubeVirt example code in PR description.

@vojtechszocs vojtechszocs force-pushed the custom-table-filters branch from 96aaafe to 5c63fb4 Compare June 17, 2019 21:12
@christianvogt
Copy link
Contributor

/lgtm

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

@spadgett , @christianvogt , @vojtechszocs , is there a way how to pass additional objects to row filters? As an example, to determine status of a virtual machine, additional objects (like pods or custom resource objects) need to be inspected.

@spadgett
Copy link
Member

/approve

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 18, 2019
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-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 18, 2019
@vojtechszocs
Copy link
Contributor Author

@spadgett , @christianvogt , @vojtechszocs , is there a way how to pass additional objects to row filters? As an example, to determine status of a virtual machine, additional objects (like pods or custom resource objects) need to be inspected.

@mareklibra This can be done as a follow-up. It seems that the VM table needs more data beyond the usual list of VM objects, so one way to solve that is to make the VM table work with composite (synthetic) objects that combine VM data and any other k8s resource data that is necessary.

@christianvogt
Copy link
Contributor

/lgtm

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, spadgett, vojtechszocs

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-merge-robot openshift-merge-robot merged commit 7a829f2 into openshift:master Jun 18, 2019
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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants