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

Increase touch area of close button in settings dialog #10145

Conversation

BalanaguYashwanth
Copy link

@BalanaguYashwanth BalanaguYashwanth commented Feb 12, 2023

Resolved bug by increasing touch area of close button in settings dialog

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)

Notes: Increased touch area and padding for close button under the settings dialog and maintaining well in terms of design and colour which suitable exactly for the settings dialog window.

Fixes element-hq/element-web#22915
Closes #9202

Type: [enhancement/defect/task]

Before -

image

After -
image

Fixes
Increased touch area and padding for close button under the settings dialog and maintaining well in terms of design and colour which suitable exactly for the settings dialog window. It fixes https Contributed by @BalanaguYashwanth


Here's what your changelog entry will look like:

🐛 Bug Fixes

Resolved bug by increasing touch area of close button in settings dialog
@BalanaguYashwanth BalanaguYashwanth requested a review from a team as a code owner February 12, 2023 09:04
@github-actions github-actions bot added the Z-Community-PR Issue is solved by a community member's PR label Feb 12, 2023
@germain-gg germain-gg added the T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems label Feb 13, 2023
@germain-gg germain-gg changed the title Signed-off-by: BalanaguYashwanth <[email protected]> Increase touch area of close button in settings dialog Feb 13, 2023
Copy link
Contributor

@germain-gg germain-gg left a comment

Choose a reason for hiding this comment

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

We're adding a wrapper HTML element for purely stylistic reasons, and that is CSS roles.

I believe we should remove the wrapper and virtually increase the touch zone using a pseudo element

button::after {
  position: absolute;
  inset: -10px;
  content: "";
}

Something of the sort will probably do the trick, but I have not tested that locally.

@BalanaguYashwanth
Copy link
Author

Hello @gsouquet,

Thanks for suggestion, I will check on it.

@BalanaguYashwanth
Copy link
Author

Hey @gsouquet,

I added to css what code you suggested but touch area is not working as expected, can you check from your side once.

Mainly i see CSS and HTML code for that close button is not structured well then i changed it. You can check demo in local of my PR you will definitely see increased touch area and aligned CSS along with HTML in UI

@BalanaguYashwanth BalanaguYashwanth requested review from germain-gg and removed request for t3chguy and florianduros February 16, 2023 04:59
@BalanaguYashwanth
Copy link
Author

BalanaguYashwanth commented Feb 17, 2023

Hello,

Anyone can help me out because @gsouquet, suggested some changes but when i tried, those are not working on UI, If anyone can guide me in some direction, so that i can improve my PR but existing PR you won't see any problems with it.

But still if you need to improve let me know...

@BalanaguYashwanth
Copy link
Author

Hey @gsouquet ,

Hope you are doing well, Can i expect any feedback.

Copy link
Contributor

@germain-gg germain-gg left a comment

Choose a reason for hiding this comment

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

I've put a very basic example showing how to increase the touch area around a button https://jsbin.com/xeqetazuqu/edit?html,css,output

I believe we should apply the same technic, rather than applying an extra HTML element as it carries zero semantic value

@BalanaguYashwanth
Copy link
Author

BalanaguYashwanth commented Mar 1, 2023

hey thanks for your reply, i am working on to match your solution because unlike other elements to add touch area, this element behaviour in different manner when applying css, i didn't stoped yet.

Working on it, soon will raise with PR with updated code, Anything you want to update me, You can update...

@BalanaguYashwanth
Copy link
Author

Hello @gsouquet,

Updated the css code please look once for increase touch area for cancel button

@BalanaguYashwanth
Copy link
Author

Hello @gsouquet,

I updated based on your inputs, If you review then it will be great

Comment on lines -478 to 479
background-color: $dialog-close-fg-color;
background-image: url("$(res)/img/cancel.svg");
background-repeat: no-repeat;
background-position: center;
background-size: cover;
cursor: pointer;
position: unset;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this change. Why is it needed, and I don't think the SVG will be tinted when changing the theme to dark or light.

Could you please revert this?

Copy link
Author

Choose a reason for hiding this comment

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

Background color also won't tined with dark or light theme because It looks same for two themes, Manipulating the background color and using as cancel icon which is not recommend

Instead using Cancel button svg which is easily maintainable.

Copy link
Contributor

Choose a reason for hiding this comment

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

This does not need to be changed, so i would request to either revert this as part of this pull request or doing it as described in https://github.com/matrix-org/matrix-react-sdk/blob/develop/docs/icons.md

width: 18px;
height: 18px;
position: absolute;
top: 10px;
right: 0;
}

.mx_Dialog_cancelButton::before{
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.mx_Dialog_cancelButton::before{
.mx_Dialog_cancelButton::before {

.mx_Dialog_cancelButton::before{
content: "";
position: absolute;
height: 50px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally we should avoid hardcoding the height but use inset or a combination of top and bottom so that the height is inferred from the parent element.

More future proof.

Copy link
Author

@BalanaguYashwanth BalanaguYashwanth May 10, 2023

Choose a reason for hiding this comment

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

As per your suggestion, Using top and bottom added the size for the tocuh area.

so you can check below which i added area of touch for this cancel button

image

@t3chguy t3chguy requested a review from germain-gg August 3, 2023 12:44
Comment on lines -478 to 479
background-color: $dialog-close-fg-color;
background-image: url("$(res)/img/cancel.svg");
background-repeat: no-repeat;
background-position: center;
background-size: cover;
cursor: pointer;
position: unset;
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not need to be changed, so i would request to either revert this as part of this pull request or doing it as described in https://github.com/matrix-org/matrix-react-sdk/blob/develop/docs/icons.md

@dbkr
Copy link
Member

dbkr commented May 16, 2024

I'm going to close this as it hasn't had any activity. Feel free to re-open if you (or anyone) wants to keep it moving forward.

@dbkr dbkr closed this May 16, 2024
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.
3 participants