Skip to content
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

Fix deletion confirmation modal #1100

Closed
garrett opened this issue Sep 14, 2022 · 7 comments · Fixed by #1451
Closed

Fix deletion confirmation modal #1100

garrett opened this issue Sep 14, 2022 · 7 comments · Fixed by #1451
Labels
bug Something isn't working

Comments

@garrett
Copy link
Member

garrett commented Sep 14, 2022

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.

Originally posted by @garrett in #1037 (comment)

@jelly
Copy link
Member

jelly commented Sep 14, 2022

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

@jelly jelly added the bug Something isn't working label Sep 14, 2022
@garrett
Copy link
Member Author

garrett commented Sep 14, 2022

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... 😅 If some of us on the team are having issues identifying things, then you can be sure our users won't know. 😉

@jelly
Copy link
Member

jelly commented Sep 15, 2022

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

@jelly
Copy link
Member

jelly commented Dec 19, 2022

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 added a commit to jelly/cockpit-podman that referenced this issue Dec 19, 2022
As cockpit-project#1100 describes, there is no need to have "Confirm.." in our title.
Follow the patternfly guidelines on dialog titles and make sure every
modal has a context.
KKoukiou pushed a commit that referenced this issue Dec 21, 2022
As #1100 describes, there is no need to have "Confirm.." in our title.
Follow the patternfly guidelines on dialog titles and make sure every
modal has a context.
@garrett
Copy link
Member Author

garrett commented Jan 17, 2023

patternfly/patternfly#1825 is an old issue about this topic. Specifically this comment states it was "recently" worked on in 2020 patternfly/patternfly#1825 (comment):

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.

@subhoghoshX
Copy link
Member

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

@garrett
Copy link
Member Author

garrett commented Sep 20, 2023

This is how it should look according to PatternFly guidelines:

image

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.

subhoghoshX added a commit to subhoghoshX/cockpit-podman that referenced this issue Oct 19, 2023
subhoghoshX added a commit to subhoghoshX/cockpit-podman that referenced this issue Oct 19, 2023
subhoghoshX added a commit to subhoghoshX/cockpit-podman that referenced this issue Nov 15, 2023
subhoghoshX added a commit to subhoghoshX/cockpit-podman that referenced this issue Nov 15, 2023
jelly pushed a commit that referenced this issue Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants