Redesign delete/remove dialog

[skobyda]

As pointed out at #640 , there are many delete/removal dialogs in c-machines which do not follow PF guidelines. Here is my initial try to adhert to the guidelines. It will likely need some rewording.

• I mostly moved identification of what we are removing from modal header, where it sometimes may not fit, into the modal body.
• Fixed some Upper Case in Network deletion modal.
• Implemented DescriptionList for more complex object which cannot be identified by just 1 value
• Specified a VM name in case something is being removed/deteted from the VM

Before

After

Before

After

Before

After

Before

After

Before

After

---

Machines: Redesign content removal dialogs

Dialogs for deleting resources on the Machines page now show details about which resources are about to be deleted.

For example: Removing host devices from the VM shows the host device with identifying information such as vendor, product, and slot number.

Another example: When deleting a storage pool, optional deletion of volumes now shows which volumes would be deleted.

[skobyda]

I'm also thinking how to redesign Storage Pool deletion dialog. As it (depending on whetever is active) contains additional message or checkbox:

[garrett]

I'm also thinking how to redesign Storage Pool deletion dialog. As it (depending on whetever is active) contains additional message or checkbox:

I don't understand. Is this what it currently looks like?

There shouldn't be a label here, except for the mandatory one for the checkbox. Which volumes would optionally be removed? It should show those.

For example, when I expand my default storage pool, I see this:

Can you even delete the storage pool without the volumes? 🤔

...What would happen to the files? And the VMs?

...And then what happens to the VMs when you delete the storage pool and the volumes?

[KKoukiou]

I won't manage to review this PR. Just from the screen shots, I noticed that the modal titles don't follow the PF guidelines. In specfiic, Confirmation modals, need to have titles with a question.
For example;

https://www.patternfly.org/v4/components/modal/design-guidelines/#confirmation-dialogs

[skobyda]

Can you even delete the storage pool without the volumes? thinking

...What would happen to the files? And the VMs?

You can delete the pool without volumes. In that case, the files (e.g. qcow2 file or raw file) are left untouched. For example, if you have a file directory storage pool, then deleting a storage pool just deletes the storage pool "abstraction" over that directory, but the directory with files is still left on the system. This can be useful if you want to move some of those files/disk elsewhere after storage pool deletion.

...And then what happens to the VMs when you delete the storage pool and the volumes?

the storage pool OR the volumes. Those are 2 different situations:

If some VM is using disk with reference as [storagePoolName]:[volumeName], then you cannot delete the storage pool:
(Dropdown Button "Delete" should be probably greyed out. This needs aria-disabled label!)

If some VM is using disk without storage pool reference, but instead is referencing the storage volume directly thru file path or some other ID without referencing storage pool, then you can delete the storage pool, but not its volumes:
(Maybe that info text shoould be a tooltip of disabled Button?)

[skobyda]

I won't manage to review this PR. Just from the screen shots, I noticed that the modal titles don't follow the PF guidelines. In specfiic, Confirmation modals, need to have titles with a question. For example;

https://www.patternfly.org/v4/components/modal/design-guidelines/#confirmation-dialogs

Then maybe just rewording those titles should be enough:
"Remove network" ->"Remove network?"
"Remove disk from VM" -> "Remove disk from VM?"

[garrett]

I've mentioned our headlines not conforming to PF titles before (regarding the "?"), but we never did it as it was a one-off and inconsistent with the rest of Cockpit.

As this is one of those PRs with sweeping changes, it makes sense to switch the titles to questions wherever it makes sense. 👍

---

If we're making these changes, we might want to also consider adding an icon for destruction too:

https://www.patternfly.org/v4/components/modal/design-guidelines/#confirm-a-destructive-action

Use a primary button to confirm a destructive action. If the action carries serious consequences, then use a danger button instead.

If an action is difficult or impossible to undo, add a warning icon beside the headline.

They're optional... But it seems to strongly suggest them above under certain destructive circumstances.

https://www.patternfly.org/v4/components/modal/design-guidelines/#icon-use-in-modal-dialogs:

Icons are optional in modal dialogs. Use or omit them as your use case requires.

Use case(s)	Usage
Warning: Cautions or warns the user of a permanent action, or that information will be deleted upon action completion	Add to confirmation dialogs or passive dialogs to indicate a higher level of urgency and importance.
Critical Warning: Indicates that an error has occured	Add to error dialogs.
Acknowledgement: Informs the user of an action or result	Add to confirmation or passive dialogs to indicate a lower level of urgency.

(But this could easily go in another PR.)

[garrett]

If some VM is using disk without storage pool reference, but instead is referencing the storage volume directly thru file path or some other ID without referencing storage pool, then you can delete the storage pool, but not its volumes:
(Maybe that info text shoould be a tooltip of disabled Button?)

Should that dialog have some sort of solution within the dialog itself?

That is, list the VMs it's in use by and perhaps have a way to detach them from the individual VMs from within the dialog?

(This probably should be addressed in an separate PR; keep this PR mainly related to reworking the modals as-is without really adding features — except for showing more information.)

[garrett]

What happens with errors? Are they inline alerts? Are those at the bottom or top? (If inline alerts, they should be at the top, like what cockpit-project/cockpit#17221 is now doing.)

I know that Machines does do a lot of error handling as alerts in the page, after a modal though, so perhaps it doesn't even need to be addressed. 😁

[skobyda]

it makes sense to switch the titles to questions wherever it makes sense. +1

Done

If we're making these changes, we might want to also consider adding an icon for destruction too

Done

If some VM is using disk without storage pool reference, but instead is referencing the storage volume directly thru file path or some other ID without referencing storage pool, then you can delete the storage pool, but not its volumes:
(Maybe that info text shoould be a tooltip of disabled Button?)

Should that dialog have some sort of solution within the dialog itself?

That is, list the VMs it's in use by and perhaps have a way to detach them from the individual VMs from within the dialog?

(This probably should be addressed in an separate PR; keep this PR mainly related to reworking the modals as-is without really adding features — except for showing more information.)

Yeah. This functionality would be way more complex, so I would leave it outside of this PR.

What happens with errors? Are they inline alerts? Are those at the bottom or top? (If inline alerts, they should be at the top, like what cockpit-project/cockpit#17221 is now doing.)

I know that Machines does do a lot of error handling as alerts in the page, after a modal though, so perhaps it doesn't even need to be addressed. grin

They were inline errors at the footer of modal in all the cases, except for disk removal, where there were alerts in page (but that was accidental, I tracked it down to my old PR where I forgot to replace page alert there with an inline modal alert, so there was no real motivation for it to be in-page alert.

So now I made it consistent, all the delete/remove dialogs now use inline alerts, and fixed them to show alert at the top of the modal:

[garrett]

it makes sense to switch the titles to questions wherever it makes sense. +1

Done

If we're making these changes, we might want to also consider adding an icon for destruction too

Done

Nice! Thanks!

---

Anything we can do about this error message?

1. We should never use the title "Failure". It should be specific to the error type.
2. What happened to the space between the alert and the rest of the modal content? (There is none here; there should be some.)
3. A CD is not a "disk". A CD is a "disc". (It's a "disc" with a C if it's a circle you can see.)
4. Should "target" really be "device"?
   For example. GNOME Disks labels the it as a "device":
   

---

...
Yeah. This functionality would be way more complex, so I would leave it outside of this PR.

Did you add the checkbox's label here? It shouldn't have that duplicate bold label to the left... It's a single opt-in checkbox. It also shouldn't have "the": "Delete volumes inside this pool". Additionally, the whole "c..." thing is odd. But I understand that part will be handled in a follow-up.

[skobyda]

2. What happened to the space between the alert and the rest of the modal content? (There is none here; there should be some.)

What do you mean? There was never a space in our AlertNotification component.
Maybe I can ad patternfly's padding-top medium: pf-u-pt-md

1. We should never use the title "Failure". It should be specific to the error type.
3. A CD is not a "disk". A CD is a "disc". (It's a "disc" with a C if it's a circle you can see.)

Fixed the modal title and error message title. However "disk" it's still present in error message, since that error message comes from libvirt itself.

4. Should "target" really be "device"?
   For example. GNOME Disks labels the it as a "device":
   ![image](https://user-images.githubusercontent.com/10246/164474645-d1aad97d-dfb1-4a6d-b6a1-a913765e23a3.png)

Term "target" is quite commonly used across libvirt, and there is reason for it, as "device" and "target" may refer to different things.

• "Device" is the identifier under which the disk is known on the host.
• "Target" specifies identifier which will be exposed to the VM. (i.e. how it's "targeted" to the VM)

Those 2 will be usually different.
For example: you have "lp0" device on your host, you attach it to the VM, but device will be likely exposed to the VM under different identifier , e.g.: "sda"
I believe if we just call it "device" , we eventually end up confusing user, and they might think they have attached "sda" of the host to the VM.

Did you add the checkbox's label here? It shouldn't have that duplicate bold label to the left... It's a single opt-in checkbox. It also shouldn't have "the": "Delete volumes inside this pool". Additionally, the whole "c..." thing is odd. But I understand that part will be handled in a follow-up.

No, the label was already there.

[garrett]

What do you mean? There was never a space in our AlertNotification component.

I meant the space in that screenshot. There should be space; alert widgets should never be slammed into something else. (And most other widgets shouldn't either.)

Disc: Nice! Thanks for fixing that (in Cockpit, where possible).

Target: Ok. That's fine.

[skobyda]

Current state of this PR:

[skobyda]

The last issues, which can be seen on screenshots above, is the deletion of storage pool, which will probably need more comprehensive redesign:

And deleteting VMs, which it seems doesn't mention the VM name anywhere in the dialog. Maybe it should. Is it okay to be mentioned in the title, or should the modal message be reworded to include the VM name?

[garrett]

Most of these changes are excellent! Thanks!

What's up with the : here in this dialog?

This still shouldn't have a label at the side, and it still doesn't mention what will be deleted. This is not great. 😢

What's up with the spacing? It should look more like this:

[skobyda]

@garrett So here is my new iteration:

Adjusted spacing in VM deletion dialog:

Storage pool with volumes will mention which volumes will be deleted:

Inactive storage pool:

Storage pool without any volumes:

This however looks a bit strange, to have just an empty modal. Or not? Not sure about this one.

[martinpitt]

@skobyda : Thanks! Please rebase, to let packit tests run.

[skobyda]

Tests green. I think it's close to landing. @garrett @martinpitt mind giving your final word?

[garrett]

Looks good! Let's merge this! 👍

[martinpitt]

Dakujem!