Create Virtual Machine #184
Conversation
openshift-ci-robot
added
the
size/L
|
|
||
| ## Step 1: Basic | ||
|
|
||
|  |
There was a problem hiding this comment.
- May as well switch to RHEL 8 in this screen and all others
- I think everything in PF4 is supposed to be capitalized sentence-style except for product names. The “Create Virtual Machine” title and quick action button would then be “Create virtual machine”. Might be worth double-checking.
- Where do help icons go? PF4’s form documentation shows the help icon next to the input label, rather than within a FormGroup. Which is correct? (I hope the documentation)
- Should include an auto-generated name here since that’s the new default behavior.
- "Create new template from configuration" should be disabled when a Template is selected unless they tweak the Flavor of that template.
- Screens use Flavour but text uses ‘merican Flavor, we should check on this 😄
There was a problem hiding this comment.
The only thing with the auto generated name is that it might not always be generated. There is a possibility that the feature has been disabled OR they are creating a VM from scratch and haven't chosen any settings yet. This is the only one I left it empty though for that reason. There will be a follow up PR to focus on this feature specifically.
|
|
||
| The latest recommended version should be pre-selected. | ||
|
|
||
|  |
There was a problem hiding this comment.
"Go to this page to learn more."
Maybe we should align with whatever the OpenShift pattern is here. I've been just putting hyperlinked "Learn more" text after the period of the previous sentence, but I'm not sure that's right either. We should figure that out.
There was a problem hiding this comment.
+1 Ya I was just adding this text to be more accessible.
There was a problem hiding this comment.
I should also say that this is just placeholder not actually what it should say. It would hopefully be different for each to allow for a more descriptive text.
|
I know this is the first of many PRs for this wizard, but to address some of the feedback from Summit, should a sixth (unclickable) "Results" step be added and the quick action button changed to something like "Review virtual machine" or "Review and create/import VM"? |
|
@andybraren ya I could go either way on it. I was planning on leaving it for the review step since we won't change much there. |
There was a problem hiding this comment.
Matt what do you think of renaming "Basic" to "General"? Although we don't have anything in patternFly around this, I think we have had similar discussions in the past. @beanh66 @lizsurette do you have any thoughts on this?
Also, I think that the field level help is "I" icons rather than "?" - I think Michael c is updating that stuff
@serenamarie125 I'm fine to switch to General. I'll confirm with Michael about that, I am referencing what we currently have in PF. We'll need to update on the PF side too. Thanks! |
There was a problem hiding this comment.
@matthewcarleton This is looking great! It looks like the first image link is broken for me, can you check on that? Also wondering if we should add a summary at the beginning of this PR with links to the 5 steps so someone could jump down quickly since the "step 1" is already quite lengthy :)
|
@beanh66 oh I'll take a look thanks! |
openshift-ci-robot
added
the
needs-rebase
matthewcarleton
force-pushed
the
create-vm
branch
from
1e23039
to
8010a9f
Compare
openshift-ci-robot
removed
the
needs-rebase
matthewcarleton
force-pushed
the
create-vm
branch
from
f38e6fe
to
d95f96f
Compare
|
Should this include the informational text that appears when clicking each info icon? Most of them are in the original doc, I believe. |
Good point @andybraren, @lizsurette and I talked about this and thought it made more sense to leave it out. That information is more relevant to the devs which they can get from the google doc. |
openshift-ci-robot
added
the
needs-rebase
matthewcarleton
force-pushed
the
create-vm
branch
from
c36e99a
to
b391f0f
Compare
openshift-ci-robot
removed
the
needs-rebase
There was a problem hiding this comment.
Other than 1 broken image (I think) and a minor typo nitpick I think this is good to merge.
WDYT @beanh66 ?
There was a problem hiding this comment.
Looks good @matthewcarleton! Just left a minor note about the empty state. Also I'm not sure if that info icon is correct since usually it's not filled in. I don't think you need to change this in the mocks as long as we implement with the correct icon though. We can check with @mceledonia.
|
|
||
| ## Launching the Wizard | ||
|
|
||
|  |
There was a problem hiding this comment.
We don't usually show two primary buttons at once. In OpenShift we leave the regular create button enabled and just don't use one in our empty states. PF4 seems to have it in the empty state but not sure it makes sense to remove or disable the other one. Want to remove it from the empty state?
There was a problem hiding this comment.
👍
Looks good @matthewcarleton! Just left a minor note about the empty state. Also I'm not sure if that info icon is correct since usually it's not filled in. I don't think you need to change this in the mocks as long as we implement with the correct icon though. We can check with @mceledonia.
yup just talked to him about this and we are moving back to the old icons. I'm updating now :)
There was a problem hiding this comment.
@beanh66 I've updated the empty state to show a secondary action in there.
Also, I've moved the "create vm" dropdown to the right, which is going to work better visually, when we add the actions dropdown it could just sit next to it. WDYT?
I agree on leaving these icons for now, there is still discussion going about going back to what we had initially or adding a custom info icon. That will just hold up this PR. We can come back and update things if we think it's need or as you said leave it to be updated in the build.
FYI @serenamarie125 @alimobrem, Per Matt's comments in the description of this PR, I am okay merging for now and revisiting form details when we are closer to 4.3 plans. |
|
|
||
| Only major version numbers are shown in the OS dropdown. The user selects one. | ||
|
|
||
|  |
There was a problem hiding this comment.
Nit: Minor version dropdown here should be 8.0 (or whatever's the latest) rather than 7.x. This applies to the next screen too.
matthewcarleton
force-pushed
the
create-vm
branch
from
6fe58ad
to
df70da0
Compare
matthewcarleton
force-pushed
the
create-vm
branch
from
df70da0
to
1cbc642
Compare
This PR is updating the Create a VM to use the patternfly 4 style. It is still undecided what the final outcome will be (modal wizard, in-page wizard, or progressive form) so the purpose of this PR is to focus more on the content.
This PR will just be updating the basic step to keep it relatively small :) Follow PRs to come that will add networking, Storage, advanced, and review.
Some changes based on feedback and further design discussion:
The name and description has been moved to the bottom to allow for a better flow for auto generated names. If the user is presented the name first they will likely fill it out only to have it replaced by import or possibly template. There will be a follow up PR for a design that outlines the auto generated name field.
The "Create Virtual Machine" button remains active at all times for better accessibility support.
Inline alerts/notifications have been replaced with standard form errors to align better with form designs.
Some screens have been removed for the sake of brevity and to keep this design focused on the wizard itself. Open to feedback on that for sure.
One update to this design will be to possibly remove import entirely from this flow and create a separate "Import Virtual Machine" action. This will follow this PR.
There are some inconsistencies (colour, spacing, borders) in the sketch library that need to be updated. I've talked to Kyle about this and hopefully we can get the library updated so these designs can reflect that.
I'm curious what others think of the indented fields for provision source, I'm not sure we still need this but didn't want to change it in case there's a good argument for it.
Closes #186