Refactor review tab for the create VM wizard #4983

pcbailey · 2020-04-08T20:43:42Z

This PR refactors the review tab for the create VM wizard.

Known To-Dos:

Finish advanced settings section

Screenshot of current status:

atiratree

+1 The review tab is starting to look more presentable :)

...tend/packages/kubevirt-plugin/src/components/create-vm-wizard/form/form-field-review-row.tsx

frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/form/form-field-form.tsx

...end/packages/kubevirt-plugin/src/components/create-vm-wizard/form/form-field-review-row.scss

...tend/packages/kubevirt-plugin/src/components/create-vm-wizard/form/form-field-review-row.tsx

...nd/packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/general-tab.scss

...end/packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/general-tab.tsx

...ckages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/networking-review.tsx

.../packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/storage-review.tsx

...tend/packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/review-tab.tsx

.../packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/storage-review.tsx

...end/packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/general-tab.tsx

frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/FlavorRow.tsx

...end/packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/general-tab.tsx

frontend/packages/kubevirt-plugin/src/k8s/requests/vm/create/common.ts

...end/packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/general-tab.tsx

...ckages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/networking-review.tsx

atiratree · 2020-04-16T14:57:37Z

frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/selectors/common.ts


 export const getCreateVMWizards = (state): Map<string, any> =>
  _.get(state, ['plugins', 'kubevirt', 'createVmWizards']);
+
+export const getOS = ({ osID, iUserTemplates, openshiftFlag, iCommonTemplates }: GetOSParams) => {


please move it to './combined.ts' or something new file please.

There are many files which use getCreateVMWizards or use it transitively. Cyclic dependency happened to me a couple of times because of this.

Do you think we should add some notice here or rename the file?

I moved getOS. I think we should both rename the file and put a warning to explain the issue. Do you want to do that in this PR or handle it as a followup?

ok let's do it in the followup

...nd/packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/advanced-tab.tsx

...end/packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/general-tab.tsx

frontend/packages/kubevirt-plugin/src/k8s/wrapper/vm/vm-template-wrapper.ts

...tend/packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/review-tab.tsx

...end/packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/general-tab.tsx

matthewcarleton

just a few comments outside our other conversation :)

matthewcarleton · 2020-04-16T18:16:12Z

.../packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/storage-review.tsx

+    { title: 'Interface' },
+    { title: 'Size' },
+    { title: 'Storage Class' },
+    { title: 'Access Mode' },


can we change this from access mode to just access, same for volume mode? It helps with the table layout when on smaller viewports

I can, but the term 'volume' has specific meanings in storage. Do you think that will be a problem?

hmmm I'm not sure. It has context with the values so maybe not? I guess we'd want to ask someone like @aglitke who would have more insight on this.

Volume is too generic. We should probably keep both intact for consistency

adding a screenshot for reference. You can see with the longer titles we get an overlay.

I think "Access" could be okay but can't think of a way to shorten "Volume Mode". I also see that "Filesystem" is wrapping. Would it be enough to shorten just the Access Mode column?

I can add the wrappable transform to make the headers wrap. Would that be sufficient, @matthewcarleton?

ya we can follow up on this if it's too much effort to get in but I'd like to see the table cleaned up a bit.

+1 to what @aglitke is saying about the access mode. That will help with the spacing. I wonder too if we can drop access from "shared access (RWX)"

Ok. Dropping "Mode" from the "Access" header. The other options that may show up in that column are Single User (RWO) and Read Only (ROX). I don't think those can be shortened and retain their meaning.

...ckages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/networking-review.tsx

...end/packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/review-tab.scss

matthewcarleton · 2020-04-16T19:09:55Z

...end/packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/review-tab.scss

@@ -5,3 +5,56 @@
 .kubevirt-create-vm-modal__review-tab-lower-section {
  margin-top: 20px;
 }
+
+.kubevirt-create-vm-modal__review-tab-section-container {


I think we can adjust the class names to be a bit shorter :)
kv-create-vm__review__section wdyt?

I'm all for it, but maybe in a future PR. This follows the convention that's used through the plugin and I think if we're going to change it, we should change it everywhere at the same time.

...end/packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/review-tab.scss

...ckages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/networking-review.tsx

frontend/packages/kubevirt-plugin/src/k8s/wrapper/vm/vm-template-wrapper.ts

...end/packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/general-tab.tsx

frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/selectors/combined.ts

frontend/packages/kubevirt-plugin/src/selectors/vm/flavor-text.ts

frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/utils.ts

atiratree · 2020-04-17T12:34:03Z

.../packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/storage-review.tsx

+    { title: 'Interface' },
+    { title: 'Size' },
+    { title: 'Storage Class' },
+    { title: 'Access Mode' },


Volume is too generic. We should probably keep both intact for consistency

frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/utils.ts

...nd/packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/advanced-tab.tsx

atiratree · 2020-04-17T13:10:36Z

...ckages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/networking-review.tsx

+    ];
+  });
+
+  return (


we should use EmptyState when there are no networks. Please see networking-tab.tsx

+ same for storage

Done, but the styling could use some tweaking. The text looks odd being centered while everything else is left-aligned. It will probably look fine on small screens, but no so much on regular/large screens. Thoughts? @matthewcarleton

yea the text is quite overwhelming, added few suggestions bellow

ya, I'd suggest left aligning and just using body text. Although it feels like with not NICs there should be an alert of some kind. WDYT @suomiy ?

Here's how it looks after the changes I made. Thoughts? @matthewcarleton @suomiy

Not sure, about alerts. It is a bit unusual situation, but the user should be quite aware what is going on - if he has VM without NICs for some reason (maybe will add them later)

also, isn't it enough to say No network interfaces? @matthewcarleton @pcbailey

...end/packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/general-tab.tsx

atiratree · 2020-04-17T16:36:47Z

...tend/packages/kubevirt-plugin/src/components/create-vm-wizard/form/form-field-review-row.tsx

-  if (!reviewValue) {
-    return null;
-  }
+  const reviewValue = value || getReviewValue(field, fieldType) || EMPTY_MESSAGE;


can you please use text-secondary className for empty message? As it would be hard to distinguish against actual description with this value.

+1 ya I suggested just normal text but I agree differentiating it will help. I also think "no description" would actually be better too.

I added the text-secondary class and changed the message to be "No" plus the field title lowercased.

...end/packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/general-tab.tsx

.../packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/storage-review.tsx

pcbailey · 2020-04-17T18:48:50Z

.../packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/storage-review.tsx

+          </Table>
+        </span>
+      )}
+      {!showStorages && (


Do you mean the design of the empty state or the other code?

.../packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/storage-review.tsx

...ckages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/networking-review.tsx

atiratree · 2020-04-17T16:57:54Z

...ckages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/networking-review.tsx

+    ];
+  });
+
+  return (


yea the text is quite overwhelming, added few suggestions bellow

...ckages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/networking-review.tsx

.../packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/storage-review.tsx

atiratree · 2020-04-17T21:11:22Z

/lgtm

openshift-ci-robot · 2020-04-17T21:11:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pcbailey, suomiy

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/packages/kubevirt-plugin/OWNERS~~ [suomiy]

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

spadgett · 2020-04-17T21:48:25Z

/hold
for #5100

spadgett · 2020-04-17T23:16:11Z

/hold cancel
/retest

spadgett · 2021-05-11T18:00:15Z

...end/packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/review-tab.scss

+  }
+}
+
+dt {


Hi @pcbailey, it looks this changes the styling of all dt and dd elements in the console, breaking the layout of our detail pages. This needs to be scoped to only the context where it is used. We should be following http://getbem.com/ naming and ideally use a class name for these styles.

We've opened https://bugzilla.redhat.com/show_bug.cgi?id=1959550

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 8, 2020

openshift-ci-robot requested review from mareklibra and rhrazdil April 8, 2020 20:44

openshift-ci-robot added the component/kubevirt Related to kubevirt-plugin label Apr 8, 2020

pcbailey force-pushed the refactor-review-tab-for-wizard branch from 76e1304 to c3438e9 Compare April 9, 2020 15:10

atiratree suggested changes Apr 9, 2020

View reviewed changes

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 9, 2020

pcbailey force-pushed the refactor-review-tab-for-wizard branch from c3438e9 to b5a18d7 Compare April 14, 2020 21:35

atiratree reviewed Apr 15, 2020

View reviewed changes

atiratree suggested changes Apr 15, 2020

View reviewed changes

pcbailey force-pushed the refactor-review-tab-for-wizard branch from b5a18d7 to bb073fe Compare April 16, 2020 00:58

atiratree suggested changes Apr 16, 2020

View reviewed changes

pcbailey force-pushed the refactor-review-tab-for-wizard branch from bb073fe to 437021c Compare April 16, 2020 15:37

matthewcarleton reviewed Apr 16, 2020

View reviewed changes

...end/packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/review-tab.scss Show resolved Hide resolved

matthewcarleton reviewed Apr 16, 2020

View reviewed changes

...end/packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/review-tab.scss Outdated Show resolved Hide resolved

matthewcarleton reviewed Apr 16, 2020

View reviewed changes

...end/packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/review-tab.scss Outdated Show resolved Hide resolved

matthewcarleton reviewed Apr 16, 2020

View reviewed changes

...end/packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/review-tab.scss Show resolved Hide resolved

pcbailey force-pushed the refactor-review-tab-for-wizard branch 2 times, most recently from 184631c to 4756db0 Compare April 17, 2020 11:57

matthewcarleton reviewed Apr 17, 2020

View reviewed changes

...ckages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/networking-review.tsx Outdated Show resolved Hide resolved

atiratree suggested changes Apr 17, 2020

View reviewed changes

atiratree reviewed Apr 17, 2020

View reviewed changes

...end/packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/general-tab.tsx Show resolved Hide resolved

pcbailey force-pushed the refactor-review-tab-for-wizard branch from ba471d6 to 47efedd Compare April 17, 2020 15:10

atiratree reviewed Apr 17, 2020

View reviewed changes

pcbailey added 3 commits April 17, 2020 14:27

Deprecate form-field-review-context

2296566

Create new general settings review view

74bd0c5

Move and refactor getOS function

37a1dee

pcbailey added 9 commits April 17, 2020 14:30

Review tab - Add empty state to network and storage sections

dfdf003

Review tab - Drop 'Mode' from Access Mode header in storage section

f93080d

Correct prettier errors

7a9a17c

Review tab - Show empty text when field is missing

c085d0a

Review tab - Use media query var for min-width

82da08b

Review tab - Modify empty text message

a85290e

Review tab - Correct getOS call in general tab

e3fce54

Review tab - Clean up empty state in storage and networking sections

6a63d96

Review tab - Fix wizard content styles

72f6b58

pcbailey force-pushed the refactor-review-tab-for-wizard branch from e7788b4 to 72f6b58 Compare April 17, 2020 18:43

pcbailey commented Apr 17, 2020

View reviewed changes

...ckages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/networking-review.tsx Show resolved Hide resolved

atiratree reviewed Apr 17, 2020

View reviewed changes

.../packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/storage-review.tsx Outdated Show resolved Hide resolved

.../packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/review-tab/storage-review.tsx Outdated Show resolved Hide resolved

Review tab - Fix empty state in storage section

57b6dc2

pcbailey changed the title ~~WIP: Refactor review tab for the create VM wizard~~ Refactor review tab for the create VM wizard Apr 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 Apr 17, 2020

pcbailey added 2 commits April 17, 2020 15:23

Review tab - Change empty state styles

a173d07

Review tab - Fix prettier errors

f4fa433

openshift-ci-robot assigned atiratree Apr 17, 2020

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

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

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 17, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 17, 2020

openshift-merge-robot merged commit dcdc0d4 into openshift:master Apr 18, 2020

spadgett added this to the v4.5 milestone Apr 20, 2020

spadgett reviewed May 11, 2021

View reviewed changes

pcbailey deleted the refactor-review-tab-for-wizard branch October 26, 2022 12:30

+                }
+              }
+              dt {

Refactor review tab for the create VM wizard #4983

Refactor review tab for the create VM wizard #4983

Conversation

pcbailey commented Apr 8, 2020 • edited

atiratree 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

matthewcarleton 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

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

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

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atiratree commented Apr 17, 2020

openshift-ci-robot commented Apr 17, 2020

spadgett commented Apr 17, 2020

spadgett commented Apr 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcbailey commented Apr 8, 2020 •

edited