PlanWizard step 2: Initial Filter & Select VMs #21

gildub · 2020-09-01T14:11:41Z

Resolves #17

konveyor-preview-bot · 2020-09-01T14:13:39Z

🚀 Deployed Preview: http://konveyor-virt-ui-pr-21-preview.surge.sh ✨

package.json

src/app/Plans/components/Wizard/FilterVMs.tsx

ibolton336 · 2020-09-08T15:43:07Z

src/app/Plans/components/Wizard/FilterVMs.tsx

+    return;
+  };
+
+  const onCheck = (event, treeViewItem) => {


Is this a potential use case for a reusable hook?

Hmm, maybe. We need to address a problem with it though, we shouldn't be mutating state in-place. All state mutations should be done via setItems or setCheckedItems or whatever we have for state setters. treeViewItem.checkProps.checked = checked; does not guarantee a re-render.

Good point - we need to make sure we are operating in a way that is react friendly.

@gildub let's make this onCheck function a no-op for now, I don't want to leave the imperative mutation here just in case we forget about it.

src/app/Plans/components/Wizard/FilterVMs.tsx

src/app/Plans/components/Wizard/SelectVMs.tsx

ibolton336

See comments. Thanks

mturley

@gildub, let me know if my feedback about the VMsOptions / TreeViewDataItem objects makes sense. Basically, I don't think we should be storing those in state at all. They're a render implementation detail. The only state we actually change is which items are selected, so let's just keep that in state.

Then, we can create an immutable set of TreeViewDataItems with checked: false on each item by traversing the API data, and then map over that to create a copy of that data with each of the checked booleans can be based on the checkedItems state array.

In order to represent the parent items being checked if the children are all checked, we can use helpers in that map to recursively look at the children when determining each checked value. See the areAllDescendantsChecked and areSomeDescendantsChecked helpers in this JSX example in the docs: https://www.patternfly.org/v4/documentation/react/components/treeview#with-checkboxes
, they use the checkedItems array to check ids of the whole subtree. See the this.mapTree method in those docs for an example of how to map over the tree and add the right checked values.

So we could do something like this, assuming we have datacenters API data in scope:

const baseOptions: TreeViewDataItem[] = [
  {
    name: 'All datacenters',
    id: 'all',
    checkProps: { 'aria-label': 'all-check', checked: false },
    children: datacenters.map((datacenter) => ({
      // Whatever we need here based on the API data structure
      name: datacenter.metadata.name,
      id: datacenter.id
      checkProps: { 'aria-label': `${datacenter.metadata.name}-check`, checked: false },
      children: ... // Another map... probably we'd refactor that into a helper we can use recursively
    }),
  },
]

And then possibly in the next PR when we add the checkbox state, we can do:

const options = baseOptions.map((option) => ({
  ...option,
  checkProps: { ...option.checkProps, checked: areAllDescendantsChecked(option) },
  children: ... // Again, map over the children recursively, so this all should probably be a helper function
});

And in fact, for checkedItems we could use the useSelectionState hook I wrote to manage adding and removing items to it instead of useState directly. https://konveyor.github.io/lib-ui/?path=/docs/hooks-useselectionstate--checkboxes

As you can see I'm coming up with this as I go, I just confused myself a little bit, let me know if you want me to try and hack this together myself as a followup PR. My point is that we should get rid of your onCheck for now, and we shouldn't keep these options in state. Let's at least make those changes in this PR and then handle the second checked map call in another PR.

src/app/Plans/components/Wizard/FilterVMs.tsx

mturley · 2020-09-08T16:20:21Z

src/app/Plans/components/Wizard/FilterVMs.tsx

+    return;
+  };
+
+  const onCheck = (event, treeViewItem) => {


Hmm, maybe. We need to address a problem with it though, we shouldn't be mutating state in-place. All state mutations should be done via setItems or setCheckedItems or whatever we have for state setters. treeViewItem.checkProps.checked = checked; does not guarantee a re-render.

mturley · 2020-09-08T16:25:10Z

src/app/Plans/components/Wizard/FilterVMs.tsx

+
+const FilterVMs: React.FunctionComponent<IFilterVMsProps> = ({ Inventory }: IFilterVMsProps) => {
+  const [activeItems, setItems] = React.useState(VMsOptions);
+  const [checkedItems, setCheckedItems] = React.useState([]);


Yeah, I notice you don't use setCheckedItems here at all, we should be using it instead of setItems and creating a const items array here by mapping over the underlying data and using checkedItems in that map to determine the checkProps.checked value.

@gildub Can we add a comment and an issue link for this work? Thx.

I opened #38 for it, but we may want to add a comment there too.

src/app/Plans/components/Wizard/SelectVMs.tsx

mturley · 2020-09-08T17:35:41Z

src/app/Plans/components/Wizard/mocks/VMs.mock.ts

@@ -0,0 +1,146 @@
+import { IVMs } from '../../../types';
+
+export const VMsOptions = [


As mentioned above, instead of putting the tree view item structure in here, let's put mock data in here of datacenters and clusters and folders the way we expect them to be in the API. Here is an example you can look at in Jeff's demo API server: https://inventory-openshift-migration.apps.cluster-jortel.v2v.bos.redhat.com/namespaces/openshift-migration/providers/vsphere/test/tree/vm (requires VPN)

Actually let's talk on Slack about this, Jeff is telling me that might not make sense:

The datacenters/clusters/folders are not the same tree.
it's either datacenter/clusters (by folder)
or
datacenter/folders/vms

src/app/Plans/types.ts

mturley · 2020-09-08T17:42:52Z

Sorry for all the thinking-out-loud, guys. The more I think about this and discuss the data structure with Jeff, the more I think maybe we should leave this PR with the VMsOptions mock data in place in that TreeViewDataItem structure, and save the creation of it based on API data for another PR. @ibolton336 thoughts?

ibolton336 · 2020-09-08T18:26:48Z

Sorry for all the thinking-out-loud, guys. The more I think about this and discuss the data structure with Jeff, the more I think maybe we should leave this PR with the VMsOptions mock data in place in that TreeViewDataItem structure, and save the creation of it based on API data for another PR. @ibolton336 thoughts?

+1 I agree - lets open an issue for that & implement in a separate PR. But the rest of the comments should be easy enough to address I think.

vconzola · 2020-09-08T18:36:23Z

@gildub Link to latest mockups for this step. Slides 29-31. Let me know if you have any questions.

src/app/Plans/components/Wizard/SelectVMs.tsx

vconzola · 2020-09-08T21:31:09Z

@mturley One thing I show in the mockups that I'd like to include if it's not too much work is the ability to expand all of the VMs table rows with a single click. Possible? What do you think?

mturley · 2020-09-08T22:31:56Z

@vconzola Yeah, that's easy with my state abstraction :) it's the same code that drives our checkboxes, so it already has a "select all" function. Where should the button go?

vconzola · 2020-09-09T00:04:42Z

@mturley Nice! I'd like it to be as shown in the mockups. Caret icon (https://fontawesome.com/icons/caret-square-right?style=regular) in the table toolbar that changes to caret down when it's selected and expands all of the rows.

mturley · 2020-09-09T00:51:33Z

Ah, gotcha. Sorry I missed that.
@gildub, if you add a button for that, you can add selectAll to your destructured object from useSelectionState and call it with a boolean for whether to expand all or collapse all.

mturley · 2020-09-09T01:02:46Z

Ah, dammit, sorry. I was thinking this was another PR.. the tree state is a bit different. Let me take a closer look tomorrow

mturley · 2020-09-09T17:55:44Z

Ok @gildub apparently I was very confused yesterday. Vince was in fact referring to the table, so my prior advice stands. You can add selectAll to your destructuring here: https://github.com/konveyor/virt-ui/pull/21/files#diff-b61187f9e8bf2d5d8a64f3f8f6dd97fbR38 and then call it in your expand/collapse-all button. You'll also want to change the icon between <AngleRightIcon /> and <AngleDownIcon /> based on areAllSelected, so add that to the destructure too (or don't destructure it like we discussed on slack).

As for how to render the expand-all button in the header of the expand-caret column that is generated by Table... hm. Dammit, maybe that needs a PF change as well. @ibolton336 what do you say we handle that as a separate issue as well?

ibolton336 · 2020-09-09T18:08:55Z

As for how to render the expand-all button in the header of the expand-caret column that is generated by Table... hm. Dammit, maybe that needs a PF change as well. @ibolton336 what do you say we handle that as a separate issue as well?

I am on board with that @mturley

mturley · 2020-09-09T18:13:55Z

Ok. I opened #35, #36 and #38 to follow up on later.

src/app/Plans/components/Wizard/FilterVMs.tsx

mturley · 2020-09-09T18:31:29Z

Since the search state is also not connected, I think we can address that as a separate issue too so we don't hold this up any longer. Opened #39 for that. We probably want to address #38 and #39 at the same time since they will involve the same traversal of the generated tree items.

src/app/Plans/components/Wizard/mocks/VMs.mock.ts

src/app/Plans/components/Wizard/SelectVMs.tsx

mturley · 2020-09-09T21:17:10Z

FYI @gildub now that #32 is merged, when you rebase this you'll need to switch your useSelectionState imports to use the lib-ui version, and in the process change the params you pass to it into an object. We should also use the new isEqual and isItemSelected parts of it as described here: https://github.com/konveyor/virt-ui/pull/32/files#r485879159

mturley

One little thing

src/app/Plans/components/Wizard/FilterVMsForm.tsx

mturley

LGTM for now, @ibolton336 I think we can address any other nitpicks as we move forward with the followup issues

Opened follow-up issues for things not addressed

ibolton336 suggested changes Sep 2, 2020

View reviewed changes

package.json Outdated Show resolved Hide resolved

gildub marked this pull request as ready for review September 3, 2020 21:19

gildub requested a review from a team September 3, 2020 21:19

gildub changed the title ~~Wizard step2~~ PlanWizard step 2: Initial Filter & Select VMs Sep 4, 2020

ibolton336 reviewed Sep 8, 2020

View reviewed changes

src/app/Plans/components/Wizard/FilterVMs.tsx Outdated Show resolved Hide resolved

ibolton336 reviewed Sep 8, 2020

View reviewed changes

src/app/Plans/components/Wizard/FilterVMs.tsx Outdated Show resolved Hide resolved

ibolton336 reviewed Sep 8, 2020

View reviewed changes

src/app/Plans/components/Wizard/SelectVMs.tsx Outdated Show resolved Hide resolved

ibolton336 reviewed Sep 8, 2020

View reviewed changes

src/app/Plans/components/Wizard/SelectVMs.tsx Outdated Show resolved Hide resolved

ibolton336 previously requested changes Sep 8, 2020

View reviewed changes

mturley reviewed Sep 8, 2020

View reviewed changes

src/app/Plans/components/Wizard/SelectVMs.tsx Outdated Show resolved Hide resolved

mturley reviewed Sep 8, 2020

View reviewed changes

src/app/Plans/components/Wizard/SelectVMs.tsx Outdated Show resolved Hide resolved

mturley reviewed Sep 8, 2020

View reviewed changes

src/app/Plans/types.ts Outdated Show resolved Hide resolved

mturley reviewed Sep 8, 2020

View reviewed changes

src/app/Plans/components/Wizard/SelectVMs.tsx Outdated Show resolved Hide resolved

gildub requested a review from ibolton336 September 9, 2020 15:38

mturley mentioned this pull request Sep 9, 2020

Add "expand/collapse all" control in the header of the leftmost column of the select VMs table in step 2 of the plan wizard. #36

Closed

mturley mentioned this pull request Sep 9, 2020

Add checkbox selection state to tree view in step 2 of the plan wizard. #38

Closed

ibolton336 reviewed Sep 9, 2020

View reviewed changes

src/app/Plans/components/Wizard/FilterVMs.tsx Outdated Show resolved Hide resolved

ibolton336 reviewed Sep 9, 2020

View reviewed changes

src/app/Plans/components/Wizard/mocks/VMs.mock.ts Outdated Show resolved Hide resolved

mturley reviewed Sep 9, 2020

View reviewed changes

src/app/Plans/components/Wizard/SelectVMs.tsx Outdated Show resolved Hide resolved

gildub added 11 commits September 9, 2020 23:24

Wizard VM selection skeleton with Treeview

e4dae3d

SelectVMs

7fde65c

FilterVMs: rename interface; Add initial click and select

588e31b

Use IVM instead IVMs

3b212d5

Use vms instead of VMs

a0acf7f

Use VMList instead of VMs

85a1acf

SelectVMs with an expandable Table

2fcbaff

Switch to useSelectionState from lib-ui

a5aa339

Fix columns for sorting

54a3e0d

Uncapitalized Plan objects properties

0e2baa4

Renamed Select and Filter VMs with Form extension

1398218

gildub requested a review from ibolton336 September 10, 2020 14:06

mturley suggested changes Sep 10, 2020

View reviewed changes

src/app/Plans/components/Wizard/FilterVMsForm.tsx Outdated Show resolved Hide resolved

FilterVMsForm: onCheck uses no-op for now

7570914

mturley approved these changes Sep 11, 2020

View reviewed changes

mturley merged commit 89623f6 into kubev2v:master Sep 11, 2020

gildub deleted the wizard-step2 branch September 14, 2020 12:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PlanWizard step 2: Initial Filter & Select VMs #21

PlanWizard step 2: Initial Filter & Select VMs #21

gildub commented Sep 1, 2020 •

edited

Loading

konveyor-preview-bot commented Sep 1, 2020

ibolton336 Sep 8, 2020

mturley Sep 8, 2020

ibolton336 Sep 8, 2020

mturley Sep 9, 2020

ibolton336 left a comment

mturley left a comment

mturley Sep 8, 2020

mturley Sep 8, 2020

ibolton336 Sep 9, 2020

mturley Sep 9, 2020

mturley Sep 8, 2020

mturley Sep 8, 2020

mturley commented Sep 8, 2020

ibolton336 commented Sep 8, 2020

vconzola commented Sep 8, 2020 •

edited

Loading

vconzola commented Sep 8, 2020 •

edited

Loading

mturley commented Sep 8, 2020

vconzola commented Sep 9, 2020

mturley commented Sep 9, 2020

mturley commented Sep 9, 2020

mturley commented Sep 9, 2020

ibolton336 commented Sep 9, 2020

mturley commented Sep 9, 2020

mturley commented Sep 9, 2020

mturley commented Sep 9, 2020

mturley left a comment

mturley left a comment

		@@ -0,0 +1,146 @@
		import { IVMs } from '../../../types';

		export const VMsOptions = [

PlanWizard step 2: Initial Filter & Select VMs #21

PlanWizard step 2: Initial Filter & Select VMs #21

Conversation

gildub commented Sep 1, 2020 • edited Loading

konveyor-preview-bot commented Sep 1, 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

ibolton336 left a comment

Choose a reason for hiding this comment

mturley left a 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

mturley commented Sep 8, 2020

ibolton336 commented Sep 8, 2020

vconzola commented Sep 8, 2020 • edited Loading

vconzola commented Sep 8, 2020 • edited Loading

mturley commented Sep 8, 2020

vconzola commented Sep 9, 2020

mturley commented Sep 9, 2020

mturley commented Sep 9, 2020

mturley commented Sep 9, 2020

ibolton336 commented Sep 9, 2020

mturley commented Sep 9, 2020

mturley commented Sep 9, 2020

mturley commented Sep 9, 2020

mturley left a comment

Choose a reason for hiding this comment

mturley left a comment

Choose a reason for hiding this comment

gildub commented Sep 1, 2020 •

edited

Loading

vconzola commented Sep 8, 2020 •

edited

Loading

vconzola commented Sep 8, 2020 •

edited

Loading