cockpit-podman icon indicating copy to clipboard operation
cockpit-podman copied to clipboard

Fix deletion confirmation modal

Open garrett opened this issue 3 years ago • 3 comments

screenshot

Issues:

  1. Title is truncated, with no way to see what it is.
    • This can be always set to wrap by changing the white-space: nowrap; to white-space: break-spaces;
    • line-clamp could help here by letting it be a specified number of lines before truncating (instead of just 1), but it's still under a -webkit- prefix and requires a bunch of wacky other bits of CSS to have it work. However, it does work cross browser in a reliable, supported manner. https://css-tricks.com/almanac/properties/l/line-clamp/ (This is incompatible with the above suggestion, which is simpler and fine as long as the line doesn't run too long.)
  2. Modal is missing content to explain what it will do and to what it will do it to.
  3. There's no need to have "Confirm ..." in the modal. That's redundant.
    • https://www.patternfly.org/v4/components/modal/design-guidelines/#confirmation-dialogs
    • I thought there were guidelines for writing modal confirmations, but I couldn't (quickly) find them. You can see in the demo that there shouldn't be a redundant title. The entire point of the modal is to ask for confirmation. I guess https://www.patternfly.org/v4/ux-writing/ux-writing-best-practices#use-positive-action-oriented-language kind of covers it in a way? (Especially in the "Be clear and concise" section.)

Originally posted by @garrett in https://github.com/cockpit-project/cockpit-podman/issues/1037#issuecomment-1246748355

garrett avatar Sep 14 '22 13:09 garrett

This deletion modal is from deletion a pod, but we should also check other dialogs.

jelly avatar Sep 14 '22 13:09 jelly

This deletion modal is from deletion a pod

Yeah, I wasn't sure, as it was truncated and didn't provide any context in the screenshot... :sweat_smile: If some of us on the team are having issues identifying things, then you can be sure our users won't know. :wink:

garrett avatar Sep 14 '22 16:09 garrett

Agreed, we should verify all of them and see if they suffer from the same issues.

jelly avatar Sep 15 '22 07:09 jelly

I've worked on 2. and 3., for 1. I am not sure what we want to do.

break-spaces doesn't help a lot:

Screenshot from 2022-12-19 11-45-35

Ideally patternfly would apply this on mobile right? Should we fill an issue?

jelly avatar Dec 19 '22 10:12 jelly

https://github.com/patternfly/patternfly/issues/1825 is an old issue about this topic. Specifically this comment states it was "recently" worked on in 2020 https://github.com/patternfly/patternfly/issues/1825#issuecomment-638405853:

Just revisiting this. In a recent PR, we added support for truncation via css line-clamp which allows you to specify the number of lines shown before truncation happens (eg, truncate after 2 or 3 lines of text). Maybe we could support that feature on the modal title, too? And potentially an opt-in for setting no truncation?

I do see line-clamp (as I suggested in this issue) as a helper utility in PF @ https://github.com/patternfly/patternfly/search?q=clamp — but it's only used for expandable sections, drawers, and alerts at the moment.

garrett avatar Jan 17 '23 11:01 garrett

Having the image tag in the title seems redundant when it's also in the body. And in the below situation it's even confusing. The title says "test-busybox:latest" will be deleted even though "test-busybox:4" is selected in the body.

Screenshot from 2023-09-13 21-37-48

subhoghoshX avatar Sep 14 '23 05:09 subhoghoshX

This is how it should look according to PatternFly guidelines:

image

  • Modal confirmation of destructive action: https://www.patternfly.org/components/modal/design-guidelines#confirm-a-destructive-action
  • Indeterminate all, nested list of checkboxes: https://www.patternfly.org/components/forms/checkbox/design-guidelines

Additionally:

  1. I removed the prefix and suffix for the image name in the title, to conserve space.
  2. I let the title wrap (instead of truncate) so it's clearer what is going on in mobile. (With the shortened name, it will fit better.)
  3. I reworded the text.

Notes: All would be selected by default. Mixed selections would have the "All" selection turn indeterminate. Everything unselected would make "All" unselected.

garrett avatar Sep 20 '23 10:09 garrett