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

ConfirmDialog: Improve styles, and use for block removal warning #58690

Closed
annezazu opened this issue Feb 5, 2024 · 13 comments · Fixed by #59645
Closed

ConfirmDialog: Improve styles, and use for block removal warning #58690

annezazu opened this issue Feb 5, 2024 · 13 comments · Fixed by #59645
Assignees
Labels
[Feature] Component System WordPress component system [Package] Components /packages/components [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@annezazu
Copy link
Contributor

annezazu commented Feb 5, 2024

Using 6.5-alpha-57531, I noticed the text for deleting a pattern category is quite squished when compared to other modals as shown below.

Screenshot of deleting a pattern category modal:
Screenshot 2024-02-05 at 12 22 38 PM

Screenshot of warning about deleting a query loop block on a template:

Screenshot 2024-02-05 at 12 23 46 PM

Can we unify this a bit more and adding a proper heading to the delete modal too for consistency? cc @WordPress/gutenberg-design for any thoughts here.

@annezazu annezazu added [Type] Enhancement A suggestion for improvement. [Feature] Patterns A collection of blocks that can be synced (previously reusable blocks) or unsynced labels Feb 5, 2024
@annezazu annezazu moved this to ❓ Triage in WordPress 6.5 Editor Tasks Feb 5, 2024
@t-hamano
Copy link
Contributor

t-hamano commented Feb 6, 2024

At least the current title feels a little pretentious to me. For example, I think it would be a good idea to indicate what we are trying to do with a title like "Delete Something".

Another approach, as shown in #58390, is to unify all buttons that perform destructive actions in red and remove the title.

@talldan talldan added [Package] Components /packages/components [Feature] Component System WordPress component system and removed [Feature] Patterns A collection of blocks that can be synced (previously reusable blocks) or unsynced labels Feb 6, 2024
@talldan
Copy link
Contributor

talldan commented Feb 6, 2024

the text for deleting a pattern category is quite squished when compared to other modals

Are you thinking the line-height of the text, width of the modal, or both? I agree it does look a bit squished, mostly the line-height, but perhaps the modal could be wider too.

The Pattern Category modal uses the general ConfirmDialog component and the line height is controlled by that component (though it also has a custom width set, not sure why):
https://wordpress.github.io/gutenberg/?path=/docs/components-experimental-confirmdialog--docs

The block removal modal doesn't use ConfirmDialog, but it really should for consistency of styles.

Then it's just a situation of updating the styles for ConfirmDialog and it should be consistent everywhere.

@talldan talldan added [Type] Bug An existing feature does not function as intended and removed [Type] Enhancement A suggestion for improvement. labels Feb 6, 2024
@jameskoster
Copy link
Contributor

ConfirmDialog could be updated to use the new(ish) size property on Modal. Maybe it can already be passed and we just need to update the instances?

I don't see any reason for the text styling to be different in these examples.

@talldan talldan changed the title Patterns: unify delete categories modal with other modals ConfirmDialog: Improve styles, and use for block removal warning Feb 7, 2024
@talldan
Copy link
Contributor

talldan commented Feb 7, 2024

It looks like the reason the pattern category dialog has a custom width is that without it the ConfirmDialog takes up the width of the content, so when there's long text it gets quite wide. I think it should really have a hard coded width of the 'small' or 'medium' modal size.

Also worth noting the responsiveness of these modals is not great, there's a lot of empty space! I think there's a bit of work to do here all around:
Screenshot 2024-02-07 at 12 17 07 pm
Screenshot 2024-02-07 at 12 19 02 pm

@jameskoster
Copy link
Contributor

If it's possible to detect, we might improve usability on small touch devices (phones) by pinning these to the bottom of the viewport and making the confirm/cancel buttons fill, e.g.:

Screenshot 2024-02-07 at 11 44 44

@richtabor
Copy link
Member

If it's possible to detect, we might improve usability on small touch devices (phones) by pinning these to the bottom of the viewport and making the confirm/cancel buttons fill, e.g.:

I like the presentation better generally, even for desktop ha :)

@jameskoster
Copy link
Contributor

Perhaps there's a way to tie to presentation of the actions to the modal dimensions. IE if the size is small (384px) or less, make the buttons fill. Agree it could work well on desktop too.

modals

@annezazu annezazu moved this from ❓ Triage to 📥 Todo in WordPress 6.5 Editor Tasks Feb 8, 2024
@jasmussen
Copy link
Contributor

If we want to change the details we should change them across, I'd avoid having one style for either viewport. I also think we can look at this holistically in a design system, rather than piecemeal.

@getdave

This comment was marked as resolved.

@talldan
Copy link
Contributor

talldan commented Feb 13, 2024

Modal for deleting a pattern category needs to be:

Other way around, it's the block removal warning that's not using ConfirmDialog, though at the same time it looks better than ConfirmDialog. So I think the plan should be:

  • Improve the styles for ConfirmDialog (line height, sizing, etc.)
  • Update the block removal warning to use ConfirmDialog instead of its custom implementation.

@annezazu
Copy link
Contributor Author

What can we do to iterate and improve for 6.5? I'd like to see the modal updated to be consistent with other modals and, ideally, we can ship some modal responsiveness improvements. @scruffian tagging you in as you very kindly recently helped with this!

@youknowriad
Copy link
Contributor

Given that RC1 is today I'm removing this one from 6.5 as non essential.

@scruffian
Copy link
Contributor

I made a PR to deal with this but I am concerned that it might be too big a change: #59645

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Component System WordPress component system [Package] Components /packages/components [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
Status: Done 🎉
Status: Done
Development

Successfully merging a pull request may close this issue.

9 participants