Bug 1871108: usability bug: the ui selections are unclear #6433

glekner · 2020-08-25T14:23:51Z

As of now, I only updated the Provision Source dropdown so we can examine the differences.
Until now, we used a component which renders the browser's native select component

Example

For us to have a more robust UI with descriptions and other props, we need to convert to the new PF Select component, its a change that could break tests, and it could take time to change all the selects in the wizard.

1 Key difference between the components is the new Select rendering the dropdown to the bottom or top is not automatic like the native select. causing it to get hidden by the wizard footer: (changing z-index didn't fix the issue)

The solution is to change the direction manually for some of the selects.

openshift-ci-robot · 2020-08-25T14:24:08Z

@glekner: This pull request references Bugzilla bug 1871108, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1871108: usability bug: the ui selections are unclear

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

glekner · 2020-08-25T14:24:19Z

@suomiy @matthewcarleton thoughts?

matthewcarleton · 2020-08-25T14:33:06Z

I guess an alternative to this would be to scroll the user down so the whole dropdown is visible.

glekner · 2020-08-26T15:30:36Z

OS and Flavor components are now separated.
Source is now between them

Some of the dropdown's content is still hidden by the footer but at least the user will know to scroll.

@suomiy @yaacov @irosenzw can you review

matthewcarleton · 2020-08-26T15:38:33Z

@glekner Is there a reason why the selection titles don't align with the design?

yaacov · 2020-08-26T15:58:32Z

...ges/kubevirt-plugin/src/components/create-vm-wizard/tabs/vm-settings-tab/vm-settings-tab.tsx

@@ -128,6 +114,34 @@ export const VMSettingsTabComponent: React.FC<VMSettingsTabComponentProps> = ({
        onProvisionSourceStorageChange={updateStorage}
        provisionSourceStorage={provisionSourceStorage}
      />
+      <Flavor


I like that we have flavor+mem+cpu together 👍

atiratree · 2020-08-26T16:17:59Z

...end/packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/vm-settings-tab/flavor.tsx

+  userTemplates: any;
+  commonTemplates: any;
+  flavorField: any;
+  operatinSystemField: any;


you can pass just the value, not the whole field

atiratree · 2020-08-26T16:19:22Z

frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/vm-settings-tab/os.tsx

@@ -42,7 +40,7 @@ import {
  NO_BASE_IMAGE_MESSAGE,
 } from '../../strings/strings';

-export const OSFlavor: React.FC<OSFlavorProps> = React.memo(
+export const OS: React.FC<OSProps> = React.memo(
  ({
    userTemplates,
    commonTemplates,


same here with the flavorField

atiratree · 2020-08-26T16:21:34Z

...end/packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/vm-settings-tab/flavor.tsx

+  commonTemplates: any;
+  flavorField: any;
+  operatinSystemField: any;
+  cloneBaseDiskImageField: any;


no need for these

glekner · 2020-08-30T09:06:06Z

Source dropdown now overlaps the wizard footer
removed unused props

can you approve @suomiy

jelkosz · 2020-08-31T10:30:04Z

@glekner One small comment: please enrich the PXE part by a mention that you need a network attachment definition available for you in this namespace or in the default one. So the message might be like:
"Boot an operating system from a server on a network. This will show up as a NIC on the Networking step. In order to PXE boot, you need to have a Network Attachment Definition available for you either in this namespace, or in the default one."

atiratree · 2020-08-31T11:40:37Z

...es/kubevirt-plugin/src/components/create-vm-wizard/tabs/vm-settings-tab/provision-source.tsx

-            <FormSelectPlaceholderOption
-              placeholder={getPlaceholder(VMSettingsField.PROVISION_SOURCE_TYPE)}
-              isDisabled={!!provisionSourceValue}
+        <Select


can you please create a new type for FormField and use it here? This will allow to pass correct values and metadata from redux to this component.

glekner · 2020-08-31T12:13:13Z

@jelkosz updated PXE text
@suomiy added PF_SELECT field type. the only supported prop I can see is isDisabled

glekner · 2020-08-31T16:36:10Z

/retest

atiratree · 2020-09-01T08:23:55Z

...es/kubevirt-plugin/src/components/create-vm-wizard/tabs/vm-settings-tab/provision-source.tsx

-            />
+          <Select
+            isOpen={isSourceOpen}
+            selections={provisionSourceValue}


we can pass this one in the FormField as well

atiratree · 2020-09-01T08:25:51Z

...es/kubevirt-plugin/src/components/create-vm-wizard/tabs/vm-settings-tab/provision-source.tsx

+          <Select
+            isOpen={isSourceOpen}
+            selections={provisionSourceValue}
+            onToggle={(isOpen) => setIsSourceOpen(isOpen)}


maybe it would be beneficial to create a single select component that has all the defaults included and stores its own state.
This will come in handy once we include multiple of these in.

jelkosz · 2020-09-02T06:48:50Z

frontend/packages/kubevirt-plugin/src/utils/strings.ts

+export const PROVISION_SOURCE_PXE_HELPTEXT =
+  'Boot an operating system from a server on a network. This will show up as a NIC on the Networking step. In order to PXE boot, you need to have a Network Attachment Definition available for you either in this namespace, or in the default one';
+export const PROVISION_SOURCE_CONTAINER_HELPTEXT =
+  'Link to a bootable operating system container located in a registry accessible from the cluster. Example: kubevirt/cirrosregistry-disk-demo. This action will not create a persistent volume claim (PVC). This will show up as a disk in the Storage step';


the correct image name is:
kubevirt/cirros-registry-disk-demo
(e.g. you miss the "-" between cirros and registry :) )

So, we need to highlight the limitations of this and reference fedora instead of cirros:
Link to a bootable operating system container located in a registry accessible from the cluster. Example: kubevirt/fedora-cloud-container-disk-demo. This action will not create a persistent volume claim (PVC). This will show up as a disk in the Storage step. The container disk is meant to be used only for read-only filesystems such
as CD-ROMs or for small short-lived throw-away VMs.';

And then, once picked, under it it needs to show the example kubevirt/fedora-cloud-container-disk-demo so the user can actually copy paste it to the text box.

jelkosz · 2020-09-02T07:01:09Z

frontend/packages/kubevirt-plugin/src/utils/strings.ts

+export const PROVISION_SOURCE_CONTAINER_HELPTEXT =
+  'Link to a bootable operating system container located in a registry accessible from the cluster. Example: kubevirt/cirrosregistry-disk-demo. This action will not create a persistent volume claim (PVC). This will show up as a disk in the Storage step';
+export const PROVISION_SOURCE_URL_HELPTEXT =
+  'Link to an operating system image via URL (HTTP or S3 endpoint). A new persistent volume claim (PVC) will be created to store this image. This will show up as a disk in the Storage step';


Maybe would be good to add an example as well, something like:

"For example, for CentOS you can visit http://isoredirect.centos.org/centos/8/isos/x86_64/ and pick a URL for an ISO which will look like: http://mirror.hosting90.cz/centos/8.2.2004/isos/x86_64/CentOS-8.2.2004-x86_64-boot.iso"

@matthewcarleton WDYT? Would something like this be helpful?

OK, that might be a bit too much and something which will be changing so we should not hardcode it here. But we could have something which will help. Something like:
"Link to an operating system image via URL (HTTP or S3 endpoint). An example might be a download link to an operating system iso you can copy from the downloads page of your favourite distribution. A new persistent volume claim (PVC) will be created to store this image. This will show up as a disk in the Storage step.".

@matthewcarleton WDYT? Would this be understandable?

Maybe:

Link to an operating system source via URL (HTTP or S3). This will show up as a disk in the storage step (backed by a persistent volume claim). Example: A operating system ISO download link you can copy from your favorite distribution.

For brevity.

can we also specify the formats?

I would explicitely add the download page there: Link to an operating system source via URL (HTTP or S3). This will show up as a disk in the storage step (backed by a persistent volume claim). Example: An operating system ISO download link you can copy from the download page of your favorite distribution.

glekner · 2020-09-02T10:43:43Z

@suomiy

added labels to ProvisionSource
created FormPFSelect component

yaacov · 2020-09-14T07:49:35Z

/lgtm
/retest

yaacov · 2020-09-14T08:54:28Z

/lgtm

openshift-bot · 2020-09-14T09:02:41Z

/retest

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

yaacov · 2020-09-14T09:05:41Z

/lgtm

yaacov · 2020-09-14T09:26:18Z

/lgtm

matthewcarleton · 2020-09-14T12:59:34Z

...packages/kubevirt-plugin/src/components/create-vm-wizard/tabs/vm-settings-tab/url-source.tsx

@@ -34,6 +36,17 @@ export const URLSource: React.FC<URLSourceProps> = React.memo(
            }
          />
        </FormField>
+        <div className="pf-c-form__helper-text" aria-live="polite">
+          For example, you can obtain the download link for {isUpstream ? 'Fedora' : 'RHEL'} cloud


can we change this to:
"Visit the Fedora cloud image[link] list and copy a url to paste into the url field here"

@glekner ^^

openshift-bot · 2020-09-14T13:48:25Z

/retest

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

openshift-bot · 2020-09-14T14:01:18Z

/retest

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

yaacov · 2020-09-14T15:11:39Z

/lgtm

openshift-ci-robot · 2020-09-14T15:11:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: glekner, yaacov

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~~ [yaacov]

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

openshift-ci-robot · 2020-09-14T18:50:25Z

@glekner: All pull requests linked via external trackers have merged:

openshift/console#6433

Bugzilla bug 1871108 has been moved to the MODIFIED state.

In response to this:

Bug 1871108: usability bug: the ui selections are unclear

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

glekner changed the title ~~Fixes 1871108: usability bug: the ui selections are unclear~~ Bug 1871108: usability bug: the ui selections are unclear Aug 25, 2020

openshift-ci-robot added the bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. label Aug 25, 2020

openshift-ci-robot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. component/kubevirt Related to kubevirt-plugin labels Aug 25, 2020

openshift-ci-robot requested review from mareklibra and yaacov August 25, 2020 14:24

glekner force-pushed the fix-1871108 branch 2 times, most recently from 28417fd to caf463c Compare August 26, 2020 15:27

yaacov reviewed Aug 26, 2020

View reviewed changes

atiratree suggested changes Aug 26, 2020

View reviewed changes

glekner force-pushed the fix-1871108 branch from caf463c to a20669f Compare August 30, 2020 09:03

atiratree suggested changes Aug 31, 2020

View reviewed changes

glekner force-pushed the fix-1871108 branch from a20669f to 6650947 Compare August 31, 2020 12:10

glekner force-pushed the fix-1871108 branch from 6650947 to eaf0b9c Compare August 31, 2020 19:22

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

atiratree reviewed Sep 1, 2020

View reviewed changes

glekner force-pushed the fix-1871108 branch from eaf0b9c to 20828f0 Compare September 1, 2020 10:47

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

jelkosz reviewed Sep 2, 2020

View reviewed changes

glekner force-pushed the fix-1871108 branch from 20828f0 to d615167 Compare September 2, 2020 10:43

openshift-ci-robot assigned yaacov Sep 14, 2020

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 14, 2020

glekner force-pushed the fix-1871108 branch from f4b6664 to c6b8e95 Compare September 14, 2020 08:33

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 14, 2020

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

glekner force-pushed the fix-1871108 branch from c6b8e95 to 03f3696 Compare September 14, 2020 09:04

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 14, 2020

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

glekner force-pushed the fix-1871108 branch from 03f3696 to 5ff2f3a Compare September 14, 2020 09:25

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 14, 2020

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

matthewcarleton reviewed Sep 14, 2020

View reviewed changes

Fixes 1871108: usability bug: the ui selections are unclear

33567bd

glekner force-pushed the fix-1871108 branch from 5ff2f3a to 33567bd Compare September 14, 2020 15:10

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 14, 2020

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

openshift-merge-robot merged commit fcf5f21 into openshift:master Sep 14, 2020

glekner deleted the fix-1871108 branch September 14, 2020 21:54

spadgett added this to the v4.6 milestone Sep 15, 2020

Bug 1871108: usability bug: the ui selections are unclear #6433

Bug 1871108: usability bug: the ui selections are unclear #6433

Conversation

glekner commented Aug 25, 2020

openshift-ci-robot commented Aug 25, 2020

glekner commented Aug 25, 2020

matthewcarleton commented Aug 25, 2020

glekner commented Aug 26, 2020

matthewcarleton commented Aug 26, 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

glekner commented Aug 30, 2020

jelkosz commented Aug 31, 2020

Choose a reason for hiding this comment

glekner commented Aug 31, 2020

glekner commented Aug 31, 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

Choose a reason for hiding this comment

glekner commented Sep 2, 2020

yaacov commented Sep 14, 2020

yaacov commented Sep 14, 2020

openshift-bot commented Sep 14, 2020

yaacov commented Sep 14, 2020

yaacov commented Sep 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-bot commented Sep 14, 2020

openshift-bot commented Sep 14, 2020

yaacov commented Sep 14, 2020

openshift-ci-robot commented Sep 14, 2020

openshift-ci-robot commented Sep 14, 2020