Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Add padding for close button in settings dialog #9202

Closed
wants to merge 7 commits into from

Conversation

gefgu
Copy link
Contributor

@gefgu gefgu commented Aug 17, 2022

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.

Fixes element-hq/element-web#22915

Signed-off-by: gefgu gefgu@hotmail.com

Checklist

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

Type: [enhancement/defect/task]

Before:
before
After:
after


Here's what your changelog entry will look like:

🐛 Bug Fixes

Signed-off-by: gefgu <gefgu@hotmail.com>
@gefgu gefgu requested a review from a team as a code owner August 17, 2022 19:07
@github-actions github-actions bot added the Z-Community-PR Issue is solved by a community member's PR label Aug 17, 2022
@gefgu gefgu changed the title Add padding for close button in dialog Add padding for close button in settings dialog Aug 17, 2022
@github-actions github-actions bot added the T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems label Aug 18, 2022
Copy link
Contributor

@SimonBrandner SimonBrandner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't seem right the button is not in the center of the clickable area...

@gefgu
Copy link
Contributor Author

gefgu commented Aug 18, 2022

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.

@SimonBrandner SimonBrandner requested a review from a team August 18, 2022 19:03
@SimonBrandner
Copy link
Contributor

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

@gefgu
Copy link
Contributor Author

gefgu commented Aug 18, 2022

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

@amshakal amshakal requested review from niquewoodhouse and removed request for a team August 22, 2022 09:51
@janogarcia janogarcia self-requested a review September 5, 2022 09:28
@janogarcia
Copy link
Contributor

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.

Copy link
Contributor

@janogarcia janogarcia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please refer to my previous comment.

@janogarcia janogarcia removed the request for review from niquewoodhouse September 5, 2022 09:32
@janogarcia
Copy link
Contributor

janogarcia commented Sep 5, 2022

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

@gefgu
Copy link
Contributor Author

gefgu commented Sep 5, 2022

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?

Copy link
Contributor

@SimonBrandner SimonBrandner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM codewise

Copy link
Contributor

@janogarcia janogarcia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems this PR is introducing (maybe reintroducing an old one?) a different close icon than the one we currently use for modal windows.

@MidhunSureshR
Copy link
Contributor

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 MidhunSureshR self-requested a review January 25, 2024 10:15
Copy link
Contributor

@MidhunSureshR MidhunSureshR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for letting this get stale, please see #9202 (comment)

@MidhunSureshR
Copy link
Contributor

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Touch area to close the settings dialog is too small.
6 participants