matrix-react-sdk icon indicating copy to clipboard operation
matrix-react-sdk copied to clipboard

Add padding for close button in settings dialog

Open gefgu opened this issue 2 years ago • 6 comments

Signed-off-by: gefgu [email protected]

Checklist

  • [ ] Tests written for new code (and old code if feasible)
  • [x] Linter and other CI checks pass
  • [X] Sign-off given on the changes (see CONTRIBUTING.md)

Notes: Add padding for close button in settings dialog. It maintains the alignment with the heading, maintains the original size of the "X" mark and follows Apple's human interface guidelines of 44x44 points for touch targets. It fixes https://github.com/vector-im/element-web/issues/22915

Type: [enhancement/defect/task]

Before: before After: after


Here's what your changelog entry will look like:

🐛 Bug Fixes

  • Add padding for close button in settings dialog. It maintains the alignment with the heading, maintains the original size of the "X" mark and follows Apple's human interface guidelines of 44x44 points for touch targets. It fixes https (#9202). Contributed by @gefgu.

gefgu avatar Aug 17 '22 19:08 gefgu

I didn't let the button in the center of the clickable area because it would lose the alignment with the heading, but if you prefer it to the center, I will work on it.

gefgu avatar Aug 18 '22 18:08 gefgu

I've requested a review from the design team to figure out how this should actually look/behave

SimonBrandner avatar Aug 18 '22 19:08 SimonBrandner

Thanks for the quick feedback! When they figure out, I'm happy to help.

gefgu avatar Aug 18 '22 19:08 gefgu

I'd suggest using a target area of 32x32px for the desktop context.

  • It's a nice trade off between not taking way too much space and complying with upcoming WCAG 2.2 recommendation for AA level (24x24px).
  • I'd only recommend using 44x44px when in a touch context (mobile viewport, webview, touch input...), or trying to comply with WCAG 2.2 AAA.

janogarcia avatar Sep 05 '22 09:09 janogarcia

As for the icon alignment and button placement I'd suggest, for now:

  • Keep the close icon centered within the 32x32px target area.
  • Place the 32x32px target area at 24px from the the modal window edges.

SCR-20220905-gzz-2

janogarcia avatar Sep 05 '22 10:09 janogarcia

I've updated the code following @janogarcia suggestion.

The close icon is centered within the 32x32 target area: button

And the target area is at 24px from the the modal window edges: outer_padding

I hope this solve the issue. What do you think @SimonBrandner?

gefgu avatar Sep 05 '22 17:09 gefgu

The playwright tests are failing due to a difference in the size of the close icons - https://e2e-9202--matrix-react-sdk.netlify.app/#?testId=e020cd10d103304ed79f-bd3d48af8331ff4cee52038c2392796d4a22. I don't think this PR intended to change the size of the close button in the first place.

MidhunSureshR avatar Jan 25 '24 07:01 MidhunSureshR

Closing this for now, feel free to reopen if you want to make the changes.

MidhunSureshR avatar Jan 29 '24 08:01 MidhunSureshR