Adding custom hook for selection on list component #5757

gnehapk · 2020-06-17T03:37:41Z

Creating a custom hook for abstracting the selection logic for a list component

frontend/packages/local-storage-operator-plugin/src/plugin.ts

frontend/packages/ceph-storage-plugin/src/components/ocs-install/node-list.tsx

frontend/packages/console-shared/src/hooks/select-list.ts

frontend/packages/ceph-storage-plugin/src/components/ocs-install/node-list.tsx

gnehapk · 2020-06-18T06:48:29Z

/assign @spadgett @bipuladh

frontend/packages/console-shared/src/hooks/select-list.ts

...kages/local-storage-operator-plugin/src/components/local-volume-set/nodes-selection-list.tsx

...es/local-storage-operator-plugin/src/components/local-volume-set/create-local-volume-set.tsx

...kages/local-storage-operator-plugin/src/components/local-volume-set/nodes-selection-list.tsx

frontend/packages/console-shared/src/hooks/select-list.ts

...kages/local-storage-operator-plugin/src/components/local-volume-set/nodes-selection-list.tsx

bipuladh · 2020-06-18T08:49:18Z

We can use the reducers as well if passing data is getting too complicated https://github.com/openshift/console/blob/master/frontend/packages/console-plugin-sdk/src/typings/reducers.ts.

spadgett · 2020-06-18T13:49:10Z

cc @jcaianirh who was going to look bulk actions

gnehapk · 2020-06-19T07:29:35Z

/assign @rawagner

gnehapk · 2020-06-23T06:31:23Z

/retest

gnehapk · 2020-06-23T07:51:04Z

/retest

rawagner · 2020-06-23T12:09:03Z

frontend/packages/console-shared/src/hooks/select-list.ts

+    onRowSelected(getAllSelectedRows(data, uniqueUIDs));
+  };
+
+  const updateSelectedRows = (rows: R[]) => {


I'd wrap this into React.useCallback

rawagner · 2020-06-23T12:11:41Z

frontend/packages/console-shared/src/hooks/select-list.ts

+      uniqueUIDs = _.xor(selectedRows, [rowData?.props?.id]);
+    }
+    setSelectedRows(uniqueUIDs);
+    onRowSelected(getAllSelectedRows(data, uniqueUIDs));


I dont see a lot of value in having getAllSelectedRows as separate function

Just inlining works fine (and is more transparent IMO)

Suggested change

onRowSelected(getAllSelectedRows(data, uniqueUIDs));

onRowSelected(data.filter((datum) => selectedRows.includes(datum.metadata.uid)));

rawagner · 2020-06-23T12:16:48Z

frontend/packages/console-shared/src/hooks/select-list.ts

+
+  const onSelect = (_event, isSelected, rowIndex, rowData) => {
+    let uniqueUIDs = [];
+    if (rowIndex === -1) {


when can rowIndex be -1 ?

I dont understand why this is not just

const selectedUIDs = isSelected ? selectedRows.push(rowData.props.id) : selectedRows.filter((uid) => rowData.props.id != uid); setSelectedRows(selectedUIDs);

why do we need to do all the uniq and xors ?

When you click select all on top of the table you will get a rowIndex of -1.

Yes, when you select checkbox that corresponds to select all nodes, the rowIndex comes as -1.

uniq is done to uniquely get the uid of the nodes as its possible that the visibleRows and selectedRows can have common uid in it.

rawagner · 2020-06-23T12:17:59Z

frontend/packages/console-shared/src/hooks/select-list.ts

+} => {
+  const [selectedRows, setSelectedRows] = React.useState<string[]>([]);
+
+  const onSelect = (_event, isSelected, rowIndex, rowData) => {


this should probably be wrapped in React.useCallback too

rawagner · 2020-06-23T12:19:08Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/create-form.tsx

@@ -153,7 +153,11 @@ export const CreateOCSServiceForm = withHandlePromise<
          kind={NodeModel.kind}
          showTitle={false}
          ListComponent={NodeTable}
-          customData={{ selectedNodes, setSelectedNodes, visibleRows, setVisibleRows }}
+          customData={{
+            onRowSelected: (selectedNodes: NodeKind[]) => {


Suggested change

onRowSelected: (selectedNodes: NodeKind[]) => {

onRowSelected: setNodes

gnehapk · 2020-06-25T02:56:18Z

@rawagner Addressed your comments. Please review.

rawagner · 2020-06-25T11:08:36Z

frontend/packages/console-shared/src/hooks/select-list.ts

+        if (isSelected) {
+          uniqueUIDs = _.uniq([...visibleRows, ...selectedRows]);
+        } else {
+          uniqueUIDs = _.uniq(selectedRows.filter((uid) => !visibleRows.includes(uid)));


you dont have to do uniq here

frontend/public/components/factory/list-page.jsx

...kages/local-storage-operator-plugin/src/components/local-volume-set/nodes-selection-list.tsx

frontend/packages/console-shared/src/hooks/select-list.ts

gnehapk · 2020-06-28T10:15:34Z

@rawagner @bipuladh Updated the PR.

frontend/packages/console-shared/src/hooks/select-list.ts

gnehapk · 2020-06-29T08:13:43Z

@rawagner Updated. Please review.

rawagner · 2020-06-29T08:14:04Z

/lgtm

openshift-ci-robot · 2020-06-29T08:14:21Z

[APPROVALNOTIFIER] This PR is APPROVED

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

~~frontend/OWNERS~~ [rawagner]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

bipuladh · 2020-06-29T06:52:16Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/node-list.tsx

@@ -50,25 +52,28 @@ const getColumns = () => {
  ];
 };

-const getSelected = (selected: NodeKind[], nodeUID: string) =>
-  selected.map((node) => node.metadata.uid).includes(nodeUID);
+const isSelected = (selected: Set<string>, nodeUID: string): boolean => selected.has(nodeUID);


Just inline use it. I don't think we need a helper function for this.

bipuladh · 2020-06-29T06:54:26Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/node-list.tsx

+  {
+    componentProps,
+  }: {
+    componentProps: { data: NodeKind[] };
+  },


Why are you destructuring componentProps?

Suggested change

{

componentProps,

}: {

componentProps: { data: NodeKind[] };

},

componentProps: { data: NodeKind[] };

Can't it be ^^?

bipuladh · 2020-06-29T06:56:35Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/node-list.tsx

-      selected: _.isArray(selectedNodes)
-        ? getSelected(selectedNodes, node.metadata.uid)
+      selected: selectedNodes
+        ? isSelected(selectedNodes, node.metadata.uid)


Suggested change

? isSelected(selectedNodes, node.metadata.uid)

? (selectedNodes.has(node.metadata.uid))

bipuladh · 2020-06-29T08:09:57Z

frontend/packages/console-shared/src/hooks/select-list.ts

+
+  const onSelect = React.useCallback(
+    (_event, isSelected, rowIndex, rowData) => {
+      const uniqueUIDs: Set<string> = selectedRows ? new Set([...selectedRows]) : new Set<string>();


isn't selectedRows being initialized as a Set?

bipuladh · 2020-06-29T08:17:37Z

...kages/local-storage-operator-plugin/src/components/local-volume-set/nodes-selection-list.tsx

-  if (_.isEmpty(rows)) return true;
-  return Object.keys(rowUIDMap).some((uid) => rows?.[uid]?.selected !== rowUIDMap?.[uid]?.selected);
-};
+const isSelected = (selected: Set<string>, nodeUID: string): boolean => selected.has(nodeUID);


Just use has

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. component/ceph Related to ceph-storage-plugin labels Jun 17, 2020

openshift-ci-robot requested review from bipuladh and zherman0 June 17, 2020 03:37

openshift-ci-robot added component/lso Related to local-storage-operator-plugin component/shared Related to console-shared labels Jun 17, 2020

gnehapk commented Jun 17, 2020

View reviewed changes

frontend/packages/local-storage-operator-plugin/src/plugin.ts Outdated Show resolved Hide resolved

afreen23 reviewed Jun 17, 2020

View reviewed changes

frontend/packages/ceph-storage-plugin/src/components/ocs-install/node-list.tsx Outdated Show resolved Hide resolved

afreen23 reviewed Jun 17, 2020

View reviewed changes

frontend/packages/console-shared/src/hooks/select-list.ts Outdated Show resolved Hide resolved

gnehapk force-pushed the list-selection-component branch from 2ba07dc to 330b61d Compare June 17, 2020 04:02

bipuladh reviewed Jun 17, 2020

View reviewed changes

frontend/packages/ceph-storage-plugin/src/components/ocs-install/node-list.tsx Outdated Show resolved Hide resolved

gnehapk force-pushed the list-selection-component branch from 330b61d to f5043b5 Compare June 17, 2020 12:37

openshift-ci-robot added the component/core Related to console core functionality label Jun 17, 2020

gnehapk changed the title ~~[WIP] Adding custom hook for selection on list component~~ Adding custom hook for selection on list component Jun 17, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 17, 2020

openshift-ci-robot assigned bipuladh and spadgett Jun 18, 2020

bipuladh suggested changes Jun 18, 2020

View reviewed changes

openshift-ci-robot assigned rawagner Jun 19, 2020

gnehapk force-pushed the list-selection-component branch 2 times, most recently from 32500da to 074ff13 Compare June 23, 2020 04:11

gnehapk force-pushed the list-selection-component branch from 074ff13 to a058348 Compare June 23, 2020 08:03

rawagner suggested changes Jun 23, 2020

View reviewed changes

gnehapk force-pushed the list-selection-component branch from a058348 to 51f9bc3 Compare June 24, 2020 03:10

rawagner suggested changes Jun 25, 2020

View reviewed changes

gnehapk force-pushed the list-selection-component branch 2 times, most recently from 15c3932 to 9ba2b39 Compare June 26, 2020 01:49

bipuladh suggested changes Jun 26, 2020

View reviewed changes

frontend/public/components/factory/list-page.jsx Outdated Show resolved Hide resolved

...kages/local-storage-operator-plugin/src/components/local-volume-set/nodes-selection-list.tsx Outdated Show resolved Hide resolved

bipuladh suggested changes Jun 26, 2020

View reviewed changes

frontend/packages/console-shared/src/hooks/select-list.ts Outdated Show resolved Hide resolved

frontend/packages/console-shared/src/hooks/select-list.ts Outdated Show resolved Hide resolved

gnehapk force-pushed the list-selection-component branch 3 times, most recently from 66ef398 to 24af09b Compare June 28, 2020 09:31

rawagner suggested changes Jun 29, 2020

View reviewed changes

frontend/packages/console-shared/src/hooks/select-list.ts Outdated Show resolved Hide resolved

frontend/packages/console-shared/src/hooks/select-list.ts Outdated Show resolved Hide resolved

frontend/packages/console-shared/src/hooks/select-list.ts Outdated Show resolved Hide resolved

Adding custom hook for selection on list component

f83ef18

gnehapk force-pushed the list-selection-component branch from 24af09b to f83ef18 Compare June 29, 2020 07:52

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 29, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 29, 2020

openshift-merge-robot merged commit ed7b51e into openshift:master Jun 29, 2020

bipuladh suggested changes Jun 29, 2020

View reviewed changes

spadgett added this to the v4.6 milestone Jun 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding custom hook for selection on list component #5757

Adding custom hook for selection on list component #5757

gnehapk commented Jun 17, 2020 •

edited

gnehapk commented Jun 18, 2020

bipuladh commented Jun 18, 2020

spadgett commented Jun 18, 2020

gnehapk commented Jun 19, 2020

gnehapk commented Jun 23, 2020

gnehapk commented Jun 23, 2020

rawagner Jun 23, 2020

rawagner Jun 23, 2020

rawagner Jun 23, 2020

bipuladh Jun 23, 2020

gnehapk Jun 24, 2020

gnehapk Jun 24, 2020

rawagner Jun 23, 2020

rawagner Jun 23, 2020

gnehapk commented Jun 25, 2020

rawagner Jun 25, 2020

gnehapk commented Jun 28, 2020

gnehapk commented Jun 29, 2020

rawagner commented Jun 29, 2020

openshift-ci-robot commented Jun 29, 2020

bipuladh Jun 29, 2020

bipuladh Jun 29, 2020

bipuladh Jun 29, 2020

bipuladh Jun 29, 2020

bipuladh Jun 29, 2020

	onRowSelected(getAllSelectedRows(data, uniqueUIDs));
	onRowSelected(data.filter((datum) => selectedRows.includes(datum.metadata.uid)));

	onRowSelected: (selectedNodes: NodeKind[]) => {
	onRowSelected: setNodes

	? isSelected(selectedNodes, node.metadata.uid)
	? (selectedNodes.has(node.metadata.uid))

Adding custom hook for selection on list component #5757

Adding custom hook for selection on list component #5757

Conversation

gnehapk commented Jun 17, 2020 • edited

gnehapk commented Jun 18, 2020

bipuladh commented Jun 18, 2020

spadgett commented Jun 18, 2020

gnehapk commented Jun 19, 2020

gnehapk commented Jun 23, 2020

gnehapk commented Jun 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnehapk commented Jun 25, 2020

Choose a reason for hiding this comment

gnehapk commented Jun 28, 2020

gnehapk commented Jun 29, 2020

rawagner commented Jun 29, 2020

openshift-ci-robot commented Jun 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnehapk commented Jun 17, 2020 •

edited