-
Notifications
You must be signed in to change notification settings - Fork 65
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.
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
Adding clone-vm design to knikubevirt folder #168
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Congrats on your first PR Ran! 🙂
Some of the legacy issues that Colleen noted in #163 (particularly 2 and 3) apply to this design as well. I also notice a few other things:
- The left-hand navigation is out of date. It's not super relevant to cloning VMs, but it would be nice to more closely align with the VM List design that Liz merged into this repo already.
- The Status cell should include text labels to the right of each icon (also shown in the VM List doc)
- The organization of the action kebabs is outdated as well (might want to double-check)
The images in the UI-vm-list
folder (within our shared Google Drive folder) include most of the updates for the above. Could you update this PR with those images and/or make updates that would resolve some of #163 as well?
Yes, congrats on the first PR!! I don't have anything additional to call out past what @andybraren has said. It would be great to hear what @beanh66 thinks after you'd made the above changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great Ran! Just a few nits :)
@@ -0,0 +1,43 @@ | |||
# Clone VM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we ok with using "VM" in place of virtual machine? @lizsurette have you thought about this at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matthewcarleton - I went back and forth when I created the initial PR with a starter list. Since it's repeated a ton I went with the shorter option, but maybe others can comment who would likely be the audience of this page in reviewing/referring back to designs... @beanh66 @cshinn @bmignano @alimobrem - This is referring to what we call the link in the main homepage to these features as well as the header title.
|
||
![VM List View Clone option in kebab menu](img/2-1-vm-list.png) | ||
|
||
Cloning is accessed from the VM List View. All item filters should be turned on by default, ensuring that all VMs are shown to the user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this read "all item filters should be turned off by default"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, fixing it in the next commit
|
||
The clone's status should change to a spinner with a completion percentage next to it. The source VM's status should change to locked while it's being cloned. The clone does not have an IP address until after it boots. | ||
|
||
If the “Transitioning” filter is disabled when the user clicks "Clone Virtual Machine", it should be re-enabled automatically to ensure that the newly-cloned VM appears in the list. This filter matches all “in between” states, including powering up, shutting down, or paused. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we replace "disabled" with "inactive" and "re-enabled" with "activated" to be more accurate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixing it in the next commit
I think this task should be reopened based on Colleen's comments on #163. Once I add text labels to the status icons, I notice something weird: Another weird thing is that the new clone does not indicate a direct relation to the original, besides being one on top of the other. The new clone has it's own action (Stop Cloning), but it is a duplication from the original. Basically, the new upcoming clone does not exist yet. We might create a better visual and textual connection, and have the two VMs share the same action. This is just an idea, don't kill me just yet. It creates a problem with the 'Created' column. But it is worth breaking alignment from the rest of the items on the list cause something is happening here. Also, Colleen mentioned that we might wanna remove that column |
Agreed, this is confusing. Sorry for missing that 🙂. Here's the original image for reference: I haven't thought through this fully, but maybe both the VM being cloned from and the almost-VM being cloned should have Looking at this again, I also wonder if the almost-VM would even be shown in this list at all until it's fully created. It might be worth confirming that with the devs.
Note that since the names of VMs could be very long, a status of "Cloning from VMname" could also be very long and get cut off. The full VM name could definitely be shown in the status popover though. "Cloning" and "Creating" might be enough to imply the connection, but feel free to consider other words/representations.
I agree that the Created column should probably be removed, and maybe the Node column too. That decision will apply to all of our mockups, so we'll need to make sure that doesn't break any other flows. Breaking alignment like this wouldn't be very easy to do from a code perspective - removing a column is much easier.
I'm not sure if this is consistent with what the rest of OpenShift does (yet?), but at the very bottom of the VM List documentation in the "States and Actions" section we proposed doing something like this:
This seems like it might be easier to maintain from a code perspective than having different action menus depending on the state of the VM, and makes it clearer to the user that some actions they're used to seeing on VMs aren't available because of that specific VM's current status. What do you think? |
looking at this quickly, I think keeping the VM that is being created from the clone should be visible. If we can ensure that the new VM will always be below the VM it is coming from we could have a status of "being cloned" and "creating clone". It would be difficult to provide longer strings but I agree with @andybraren that we can offer further details in the popup. |
I do think we need to simulate a 'Copy Paste' behavior. Adding another idea: If we do that, the origin VM item can go either of the two following options: A. Adding unique action on the Kebab flyout menu. B. Disabling the kebab, and adding an action under the status. WDYT? |
@Ranelim I am more in favour of adding the "Cancel Cloning" to the kebab in place of where the "clone" action is. We can rely on users looking there for actions. If we disable the kebab entirely it could be confusing because you don't know why it's disabled. The "Off for cloning...24%" reads a bit strange to me. I like what @andybraren suggested with the in progress icon. Does "Being Cloned" make sense? We have the ability to offer more information here too via the popup dialog. I don't think we need to overload the status here if we are going to provide the same information more efficiently via that notification above the list. |
Good discussions in this PR 🙂
Good point. Cloning could take several minutes depending on the size of the VM's disk/s, but I wonder if this is even technically possible. It's probably worth asking devs whether this is a use case we should support. If it is, then clicking the "Stop cloning" action might have to list all of the VMs being cloned in a modal, which complicates things.
This is a little tricky. In that doc specifically, the blue and red boxes you see may turn into parts of a Dashboard-like "Health" card for that specific object in the future. That design is still being worked on. There are existing OpenShift designs that use blue boxes (one example), but they're typically used for educational text or to guide the user to take one specific action. Maybe an OpenShift Designer can chime in, but using them on a Table page to emphasize "a process currently undergoing for this page" might be unconventional. There are a few other things I'd wonder about. If 4 VMs are cloning, would 4 blue boxes appear? When those boxes disappear and the table starts to jump up, would users find that annoying? Is cloning a VM such an important action that users would expect to see it emphasized in Table pages? I agree that a way to see all ongoing, user-activated operations like VMs cloning, Hosts provisioning, Cluster operators updating, etc. is important, but I wonder if a Dashboard "Activity" card located somewhere else would be the most appropriate place to see the progress of all of those things.
Is "Stop cloning" a common action for a user who just initiated that process? I would assume not, and since it's a (potentially) destructive action I'd be okay with putting it an additional click away within the action kebab.
Some of our Metal3 designs simplify the truth in a similar way. A VM might be "Off", but the user probably only cares that the clone action they started is "In Progress" and still happening, so that's the status that we show. Maybe "Cloning (50%)" for the original VM and "Creating (50%)" for the new one (since it kinda is)? I agree that status popovers (maybe even with a progress bar) could help here, and could even include a "Cancel Clone" action. |
@Ranelim I really like this idea. I think we could accomplish this by aligning with some previous design patterns that have been established. What if we had the status text "Cloning (50%)" as @andybraren suggested and when clicked the dialog would provide the progress bar, name of new VM and name of cloned VM as well as the cancel action? @andybraren what you do think? |
I agree. It might be worth researching to see what happens in other products, but this seems more straightforward from both a UX and a code perspective. I'm fine with not showing the "ghost" VM until it actually finishes.
We do use confirmation modals for other actions and using one here seems very appropriate. The "Stop Cloning" primary action button should be blue in your draft mockup.
I agree, the new status popover pattern shown here seems like the right approach and would basically contain what your second screen shows (maybe with a little more descriptive text). The "Stop Cloning" action would be the blue action in the bottom-left corner of the popover. One thing I'm uncertain about is what to call the action that stops/cancels the clone operation. Our original screen's kebab action was "Cancel Clone" to be consistent with "Cancel Migration" and "Cancel Snapshot". But if we show a confirmation modal, the two buttons at the bottom will be "Cancel" (which closes the modal) and "Cancel Clone". That could be slightly confusing. Here's the old screen: Should we change these actions to "Stop Cloning", "Stop Migrating", and "Stop Snapshot"? @matthewcarleton I think I remember discussing this a while back, but we may as well clean this up now. |
@andybraren I don't feel too strongly about replacing cancel with stop but agree that whatever approach we take should be consistent everywhere. @Ranelim how do you feel? |
About the label: About that modal: @andybraren About the modal on the second screen, do we use a "Close" button too, or rely only on the 'X'? |
I went ahead with "stop". "Cancel cloning" looks funny in the modal. @matthewcarleton, @andybraren, WDYT? R we closer to a now commit? |
@Ranelim - Looks great. Very close to ready in my opinion. A few nitpicks on the "Stop Cloning" modal...we should bring this in alignment with the current OpenShift styling I think. Here's an example from trying to Delete a Pod: Also a few minor typos... "Prosess" -> "Process" and "Stoping" -> "Stopping". Last step would be to get a +1 from @beanh66 :) |
"Stop" seems fine to me. Better than "Abort", and although "Stop" could be interpreted as "Pause" it's still a better option than having two buttons with the word "Cancel" in them.
A "Cancel" button is always included I believe, but it does the same thing as clicking the X. I see your latest screen includes it. Sorry to be so nitpicky, I know it gets annoying (especially in GitHub where changes are harder), but we're definitely close! Just a few more things based on your latest screens. 🙂
|
Oh, I’m now realizing that this PR and the VM List doc also use the old lightning bolt icon for Running - we now use the |
@Ranelim this is looking really close!
I'd say we don't want percentage in there for how it complicates things in general like you're pointing out and also the complexity of building the thing. It would have to be dynamically updating as it progressed which seems awkward in this scenario. Should we leave all list view updates for another PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Ranelim! I left a minor comment around toast notifications. Let me know what you think.
|
||
![Clone VM started](img/2-4-0-vm-list-cloning.png) | ||
|
||
If the cloning process starts successfully, the newly-cloned VM will appear in the list and a toast notification will confirm that the cloning process has begun. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mentioned this in another PR from @yfrimanm as well but in general we don't use toasts in the console to indicate a process has started when that process was just activated by the user. We used to have these type of toasts in 3.x and due to user feedback have gotten away from notifying users of their own actions. For a process that is not instantaneous, it's still valid to notify via toast when it completes. Toasts are not currently in the console so if we add them, they should use PF4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. As long the user has some feedback that something happened, we should remove them.
- They are not ideal for displaying undergoing processes
- Duplication to the item status on the list
- underdefined and unused in our current system.
…uasts. Adding clickable status
@andybraren Please note that I changed the clone source VM actions from only 'Run' and 'Delete' disabled to all, except 'Clone' which toggles to 'Stop Cloning' (In case it was under everyone's radar on previous iterations). I think it clears out some complications, but also creates new ones. The user can't create more than one clone at a time. Without adding a unique for cloning action, we should consider adding 'Number of clones' on the 'Clone VM' modal. It means that renaming them probably occur only after the cloning process is done. We can figure all this out on the next PR. |
Makes sense to me. New confirmation modal looks good too except one nit: "delene" to "delete".
Leaving this for a future PR sounds good, and I agree that the design for this capability is probably straightforward. Thinking about and mocking up a possible future feature is fine, but I'd want to confirm that this is A) possible on the technology side with little overhead or ongoing maintenance and B) a common user need and user-driven feature request. Googling around I see some small forum threads from ~2009 about creating multiple clones at once with some people noting that there's a performance impact of doing so that would need to be considered on the implementation side. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM @Ranelim!
Only one minor comment on maybe changing to a destructive button for stop clone. Let me know what you think and I'm happy to merge whenever you feel it's ready :)
|
||
While the clone is in progress, the source VM's actions are all disabled, expect the clone which is doggled to `Stop Cloning`. | ||
|
||
![Stop Cloning confirmation modal](img/3-4-3-vm-list-cloning-source-stop.jpg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ranelim I think the Stop Cloning button is a destructive action so it should be red.
@Ranelim - after the merge I noticed there isn't a link to this from the main page. Would do a followup PR to update the Readme file to include a link to this new documentation? Should be a very quick merge :) |
Sorry should have caught that before merging! |
Adding 'clone-vm' workflow. Cloning a VM allows the user to quickly create an identical copy of a virtual machine while powered off.
Summary:
@openshift/team-ux-leads