kubevirt: add ResultTab to CreateVMWizard #2568

atiratree · 2019-09-03T10:50:31Z

depends on:

kubevirt: add ReviewTab to CreateVMWizard #2567

atiratree · 2019-09-03T10:51:42Z

/hold

atiratree · 2019-09-30T11:24:34Z

/hold cancel

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

mareklibra · 2019-10-04T10:03:02Z

.../packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/result-tab/result-tab-row.tsx

+import * as classNames from 'classnames';
+import { Expandable } from '@patternfly/react-core';
+
+import './result-tab-row.scss';


I have already raised following question in another PR but it was not answered.

Throughout the code, I can see import of SCSS

like this one

like this one but filename is prefixed with underscore _ (like it should be used via SCSS @import but is not)

single hierarchy built via SCSS @importinitiated by style.scss for both @console/internal and kubevirt-plugin

Both cases have there pros and cons (encapsulation vs. single understandable hierarchy).
Considering we are developing an end-user application and not a reusable library, the case of a single hierarchy makes more sense to me. But I do not have a strong opinion on that as long we stay consistent.

@vojtechszocs , @rawagner , @suomiy , wdyt?

my preference is to have it at the same place as the tsx file and maybe have one place for common css files

This is not a blocker for this PR, so let's discuss that face to face. The point is just to be consistent.

mareklibra · 2019-10-04T10:05:39Z

...packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/result-tab/result-tab-row.scss

+}
+
+.kubevirt-create-vm-modal__result-tab-row--error {
+  background-color: #ffe6e6;


How did we get these colours? Aren't they defined by patternfly?

they were taken from pf3 notification error

We don't have a PF4 colour for a light red but we probably should. I can see about getting that added.

fixed the dark red, let's proceed with this one and patch it later once the light red is available

atiratree · 2019-10-04T12:47:07Z

screenshots

atiratree · 2019-10-04T12:52:40Z

@matthewcarleton

...packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/result-tab/result-tab-row.scss

matthewcarleton · 2019-10-04T20:22:20Z

...packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/result-tab/result-tab-row.scss

@@ -0,0 +1,17 @@
+.kubevirt-create-vm-modal___result-tab-row-container {


Are we writing specific namespace based classes for Kubevirt? I'm trying to learn more about the general css approach, is there any documentation on this ?

yes, since this component is kubevirt specific. The components are moved to core once they are used by multiple plugins. (there is some doc here)

+1 thanks for that link!

matthewcarleton · 2019-10-04T20:46:35Z

What if we did highlighting in the YAML to show the error? (forgive this YAML content I grabbed it from a pod :) )

Instead of making the entire block red could we highlight that portion that has the error?
The future implementation of this will allow the user to go back right? Could they also edit the YAML here?
Can we use the https://www.patternfly.org/v4/documentation/react/components/expandable/ here?

atiratree · 2019-10-07T08:27:07Z

What if we did highlighting in the YAML to show the error? (forgive this YAML content I grabbed it from a pod :) )

Instead of making the entire block red could we highlight that portion that has the error?

We will show the appropriate errors in the list in the future as showcased here

Nevertheless there might be errors where location in the yaml might be unclear.

The future implementation of this will allow the user to go back right? Could they also edit the YAML here?

Yup we are planning to do that. Sure we could also add YAML editing here.

Can we use the https://www.patternfly.org/v4/documentation/react/components/expandable/ here?

It is already used here.

mareklibra · 2019-10-07T09:04:01Z

/lgtm

openshift-ci-robot · 2019-10-07T09:04:24Z

[APPROVALNOTIFIER] This PR is APPROVED

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

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~frontend/packages/kubevirt-plugin/OWNERS~~ [mareklibra,suomiy]

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

openshift-bot · 2019-10-07T09:36:11Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot requested review from pcbailey and spadgett September 3, 2019 10:50

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. component/kubevirt Related to kubevirt-plugin component/shared Related to console-shared labels Sep 3, 2019

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

atiratree force-pushed the kubevirt.createVMWizard.reviewResults branch from 6914d22 to 52b3730 Compare September 3, 2019 14:22

atiratree mentioned this pull request Sep 10, 2019

kubevirt: refactor createNicRow to NICModal #2670

Merged

1 task

atiratree force-pushed the kubevirt.createVMWizard.reviewResults branch from 52b3730 to 19eee49 Compare September 18, 2019 12:54

atiratree changed the base branch from master to master-4.3 September 18, 2019 12:54

atiratree force-pushed the kubevirt.createVMWizard.reviewResults branch 3 times, most recently from 11aa67b to de75be1 Compare September 24, 2019 13:43

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 24, 2019

atiratree changed the base branch from master-4.3 to master September 26, 2019 09:30

atiratree force-pushed the kubevirt.createVMWizard.reviewResults branch 3 times, most recently from bcbd677 to 1b820cc Compare September 27, 2019 12:42

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 27, 2019

atiratree force-pushed the kubevirt.createVMWizard.reviewResults branch 2 times, most recently from 97f3e1d to 80ac2ec Compare September 30, 2019 09:17

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

atiratree force-pushed the kubevirt.createVMWizard.reviewResults branch 2 times, most recently from 0e71be1 to ebf41b8 Compare October 3, 2019 12:37

mareklibra reviewed Oct 4, 2019

View reviewed changes

atiratree force-pushed the kubevirt.createVMWizard.reviewResults branch from ebf41b8 to 4aef50b Compare October 4, 2019 13:10

matthewcarleton reviewed Oct 4, 2019

View reviewed changes

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

matthewcarleton reviewed Oct 4, 2019

View reviewed changes

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

matthewcarleton reviewed Oct 4, 2019

View reviewed changes

kubevirt: add ResultTab to CreateVMWizard

630095c

atiratree force-pushed the kubevirt.createVMWizard.reviewResults branch from 4aef50b to 630095c Compare October 7, 2019 08:16

openshift-ci-robot assigned mareklibra Oct 7, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 7, 2019

openshift-merge-robot merged commit 3bd6307 into openshift:master Oct 7, 2019

spadgett added this to the v4.3 milestone Oct 9, 2019

		@@ -0,0 +1,17 @@
		.kubevirt-create-vm-modal___result-tab-row-container {

kubevirt: add ResultTab to CreateVMWizard #2568

kubevirt: add ResultTab to CreateVMWizard #2568

Uh oh!

Conversation

atiratree commented Sep 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atiratree commented Sep 3, 2019

Uh oh!

atiratree commented Sep 30, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthewcarleton Oct 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atiratree commented Oct 4, 2019

Uh oh!

atiratree commented Oct 4, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthewcarleton commented Oct 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atiratree commented Oct 7, 2019

Uh oh!

mareklibra commented Oct 7, 2019

Uh oh!

openshift-ci-robot commented Oct 7, 2019

Uh oh!

openshift-bot commented Oct 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

atiratree commented Sep 3, 2019 •

edited

Loading

matthewcarleton Oct 4, 2019 •

edited

Loading

matthewcarleton commented Oct 4, 2019 •

edited

Loading