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

Modal: Update size and positioning of Close button #65102

Closed
mirka opened this issue Sep 5, 2024 · 5 comments · Fixed by #65131
Closed

Modal: Update size and positioning of Close button #65102

mirka opened this issue Sep 5, 2024 · 5 comments · Fixed by #65131
Assignees
Labels
Needs Design Feedback Needs general design feedback. [Package] Components /packages/components [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.

Comments

@mirka
Copy link
Member

mirka commented Sep 5, 2024

What problem does this address?

Currently, the Modal component renders a Close button in the old 36px size, and at a slight offset relative to the modal content width.

Modal header

In accordance with the new sizing scheme, we need to change the button size to either the 40px or 32px variant.

What is your proposed solution?

  • I think the 32px variant ("compact") would be the better fit for this use case.
  • Should we reconsider the offset as well? I feel like it could be flush with the content width and not be weird, especially with the more compact size.

Pinging @WordPress/gutenberg-design for direction, and to note that I'm seeing somewhat conflicting mockups around the modal close button in the Figma (Buttons exploration that explicitly uses a 40px variant, and the Modal mockups that use a raw icon and not an actual Button component).


When implementing this change, remember that this has consistency implications with buttons that can potentially be in the headerActions slot next to the close button. This may warrant a Dev Note. In Gutenberg, there are some Modals that inject a "Fullscreen mode" icon button here, so we'll need to update the sizes for that accordingly.

@jasmussen
Copy link
Contributor

Many hidden details in getting a close button right, including position, optical size, hit area, footprint lining up with content and headings, etc. Enough that I'd think it'd be worth, system-wise, to do an audit of all close buttons and find some rules. In the mean time, I'd do the smallest possible change to address the legacy button size here. Whether that's 32px or 40px, either seems fine for the modal.

@ciampo
Copy link
Contributor

ciampo commented Sep 6, 2024

Agreed. I think that what Lena proposed above would be the smallest change

  • I think the 32px variant ("compact") would be the better fit for this use case.
  • Should we reconsider the offset as well? I feel like it could be flush with the content width and not be weird, especially with the more compact size.

@jameskoster
Copy link
Contributor

One easy way to achieve alignment is to use a title line-height that matches the button size, and apply equal padding to the modal itself.

As an example this is 24px padding, small button, and a title with 24px line-height ('Heading XL' according to the proposal in #64340 (comment)):

Screenshot 2024-09-06 at 15 30 10

I'm unsure about tap target requirements as it's often easier to close modals in other ways; 'Cancel' button, press esc, click outside the modal.

One other detail is to avoid overlap between text in a title-less modal and the close button. Though that perhaps falls into ConfirmDialog territory and may warrant a different design.

@mirka mirka self-assigned this Sep 6, 2024
@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Sep 6, 2024
@mirka
Copy link
Member Author

mirka commented Sep 6, 2024

Thanks for the design direction! Making the padding and line-height equal sounds good. If it's ok though, I'd like to keep this iteration to just the button size and offset, to minimize the testing surface. Here's a PR with the proposed changes: #65131.

@afercia
Copy link
Contributor

afercia commented Nov 5, 2024

As noted in #66277

It is worth reminding that the WCAG Target Size (Minimum) requires a size of at least 24 by 24 CSS pixels. That's the minimum though. There's no reason not to make these buttons bigger, when possible. The editor should aim to provide the best possible experience and not be limited to meet the minimum requirements.

I disagree with the change in #65131 as it makes the X close button target size unnecessarily too small. The 32px variant would have been better.

I have to note that the target size is an important accessibility feature and neither this issue nor the related PR #65131 have any accessibility label or any consideration about potential impact on accessibility, which is, honestly, disappointing.

I will propose to change this size to 32 pixel in #66277. For the future, I'd like to suggest to refrain from making changes that introduce, de facto, a reduced level of accessibility without discussing it with specialists. This process is far from ideal as we'll need now to change it again in a future release and publish one more dev note and ask extenders to adapt again. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Design Feedback Needs general design feedback. [Package] Components /packages/components [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants