machines: Detach disk

[KKoukiou]

Rebased on top of https://github.com//pull/9240, cannot be merged before it.
update: 9240 is already merged so this is ready for review.

[KKoukiou]

@andreasn @garrett I am implementing a new feature in machines package. Specifically, in the Disks sub tab I am adding the [-] button to allow user to detach disks from VMs.
What do you think about the button choice? Is it a problem that the [Add Disk] button is different than the [-] button for removing disks?

[garrett]

I was looking for examples of this pattern in Cockpit, and found a bunch.

...but we're consistently inconsistent for both the add and remove buttons. I'm looking through PatternFly at the moment to see if there guidelines on what should be used (and so far, I haven't found anything).

I took a bunch of screenshots when looking around:

User account SSH keys

Applications

Storage

Firewall

[garrett]

I have scoured PatternFly.org and still haven't found anything relevant.

Here are interpreted rules from what we're doing on Cockpit:

• A + button by itself is only used when in a grey header in a box.
• A - button is used for removing something.
• A trashcan is used instead when something is deleted, instead of just removed. (It's basically remove and delete.)
• The Machines page uses a link for adding. (FWIW: I'm of the opinion that it should probably be a button, not a link.)

So, unless @andreasn has a difference of opinion, I think having an "Add" button spelled out and "-" buttons is probably correct.

We should probably test these patterns.

---

TL;DR: Yes, I think it's probably fine as-is.

[garrett]

Firstly: It's hard to see it without the HTML + CSS, and I can't really get this working on my machine to test it locally. 😞

So I'll elaborate what we talked about in IRC with a few other details worth mentioning:

• Vertical alignment seems off for items in the table rows. It is probably due to additional padding for one (or more) of the elements inside the row. Otherwise, it could be vertical-align not being set to baseline.

• The horizontal alignment could probably also be improved by left-aligning the text. If "File" is a label and could also be a "URI", then you could either put that in a <td> and use a colspan=2 on the corresponding "Source" <th> heading or add a min-width of 5em or so to the label. Both techniques should ensure the path (or URI? or anything else it could be?) are the same width. An advantage of the colspan technique is not requiring extra CSS and having the items line up perfectly without any assumptions.

• I'd suggest removing the label or whatever element the "File" is enclosed within and using the th colspan to offset it from the path, with both the "File" and path in separate <td>s.

• Also, the horizontal separator lines between the items seem a bit dark. 🤷‍♂️ (But perhaps I'm just being picky here?)

Here's a modified version of your screenshot that illustrates the above suggestions:

---

It would be nice if this could be fixed in this PR, but I'm happy to have this addressed in a second PR too if that makes things easier.

[KKoukiou]

@garrett I think it's better to open separate issue for your comments above, because all are relevant to whole disks rows, however this PR is only adding the [-] button for each row.
So all 4 comments, are not relevant to this specific change. I 'll be glad to fix these in seperate PR.

[KKoukiou]

@martinpitt it's ready for review again!

[martinpitt]

Looks great now, thanks!