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.

Sign up for GitHub

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

Jump to bottom

kubevirt: make VM Wizard errors more explicit #3585

Merged
merged 2 commits into from Dec 3, 2019
Merged

kubevirt: make VM Wizard errors more explicit #3585

merged 2 commits into from Dec 3, 2019

Conversation

atiratree
Copy link
Member

@atiratree atiratree commented Nov 26, 2019 •
edited

depends on:

VM Wizard changes:

General

describe error form fields inside the footer

  • mention all fields which include an error
  • once those are fixed mention empty required fields
  • fix disk/pxe boot source positioning

fill

fill-a

fill-b

Cloud Init

  • make discard dialog more explicit
    clinit

clinit2

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 26, 2019
@atiratree
Copy link
Member Author

@matthewcarleton

@openshift-ci-robot openshift-ci-robot added the component/kubevirt Related to kubevirt-plugin label Nov 26, 2019
@atiratree atiratree force-pushed the kubevirt.vmWizardEnhancements2 branch 2 times, most recently from 728c51b to c8312b5 Compare November 26, 2019 19:07
@matthewcarleton
Copy link
Contributor

matthewcarleton commented Nov 26, 2019 •
edited

@suomiy Looking great!
I wonder for errors where multiple fields are required if we should break it down further
Screen Shot 2019-11-26 at 3 39 27 PM
I have some comments on the boot source but I'm not sure we need to worry about this since we are moving this to the advanced section anyway.
For the cloud-init can we have the title "data loss confirmation" and append "Are you sure you want to want to take this action" and then the action can be "Confirm" rather than "yes". I honestly would love this to be Discard custom values for the action, WDYT?
Would the content also make sense like this?
When using the Cloud-init form custom values can not be applied and will be discarded. The following fields will be kept: Hostname, SSH-authorized-keys.
It might help the user understand what they've done to get this modal.

@atiratree atiratree force-pushed the kubevirt.vmWizardEnhancements2 branch 2 times, most recently from d41585e to 6031458 Compare November 28, 2019 12:17
@atiratree
Copy link
Member Author

@suomiy Looking great!
I wonder for errors where multiple fields are required if we should break it down further

like this?

g1

g2

g4

I have some comments on the boot source but I'm not sure we need to worry about this since we are moving this to the advanced section anyway.
For the cloud-init can we have the title "data loss confirmation" and append "Are you sure you want to want to take this action" and then the action can be "Confirm" rather than "yes". I honestly would love this to be Discard custom values for the action, WDYT?

I think Confirm should be enough. We are already asking for confirmation and telling the user that the data will be discarded. It is not a hard dangerous action.

Would the content also make sense like this?
When using the Cloud-init form custom values can not be applied and will be discarded. The following fields will be kept: Hostname, SSH-authorized-keys.
It might help the user understand what they've done to get this modal.

done

c1
c2

@matthewcarleton
Copy link
Contributor

@suomiy yes! Looks great!
Two small things:
For inline alerts the icon should be aligned to the top not centered, so there might be some issue with the implementation there.
For the Data Loss modal
I like the Data Loss Confirmation as the title.
We should move the question below the When using the cloud-init form.... content so they read that last and then Confirm what the question asks.

@atiratree atiratree force-pushed the kubevirt.vmWizardEnhancements2 branch from 6031458 to 51e63ac Compare November 29, 2019 12:53
@atiratree
Copy link
Member Author

fixed

tt

aa

@atiratree
Copy link
Member Author

/retest

@matthewcarleton
Copy link
Contributor

matthewcarleton commented Nov 29, 2019 •
edited

@suomiy much better!
Is it possible to break the "Are you sure want to take this action?" line to the next line? It reads a bit nicer.
Screen Shot 2019-11-29 at 11 31 23 AM

@atiratree atiratree force-pushed the kubevirt.vmWizardEnhancements2 branch from 51e63ac to 7b137e9 Compare December 2, 2019 16:56
@atiratree
Copy link
Member Author

fixed

aa

/assign @mareklibra

@atiratree
Copy link
Member Author

/retest

mareklibra
mareklibra reviewed Dec 3, 2019
View reviewed changes
...t-plugin/src/components/create-vm-wizard/redux/initial-state/cloud-init-tab-initial-state.ts Outdated

export const getCloudInitInitialState = () => ({
export const getCloudInitInitialState: () => StepState = () => ({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we introduce new type for these initalState functions? It will improve readability of the code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed, + added additional types

@suomiy
kubevirt: describe the form fields in the error inside the footer
04c4186 
- mention all fields which include an error
- once those are fixed mention empty required fields
- fix disk/pxe boot source positioning
@suomiy
kubevirt: make discard dialog more explicit in VM wizard cloud init
4b7d256
@atiratree atiratree force-pushed the kubevirt.vmWizardEnhancements2 branch from 7b137e9 to 4b7d256 Compare December 3, 2019 12:12
@atiratree
Copy link
Member Author

/retest

@mareklibra
Copy link
Contributor

/lgtm

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

[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

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

@spadgett spadgett added this to the v4.4 milestone Dec 3, 2019
@atiratree
Copy link
Member Author

/retest

@openshift-bot
Copy link
Contributor

/retest

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

@atiratree
Copy link
Member Author

/retest

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-merge-robot openshift-merge-robot merged commit d1359cb into openshift:master Dec 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mareklibra mareklibra mareklibra left review comments

@jelkosz jelkosz Awaiting requested review from jelkosz

@pcbailey pcbailey Awaiting requested review from pcbailey

Assignees

@mareklibra mareklibra

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/kubevirt Related to kubevirt-plugin lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v4.4
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@atiratree @matthewcarleton @mareklibra @openshift-ci-robot @openshift-bot @spadgett @openshift-merge-robot @suomiy