Skip to content

Conversation

@crisbeto
Copy link
Member

@crisbeto crisbeto commented Jun 9, 2021

This is a reimplementation of #22846 which includes a fix for RTL layouts.

Adds some styles to ensure that the MDC-based button always has a touch target of at least 48px. Note that I decided against using MDC's built-in touch target class, because it would've added a margin on the button host node.

Fixes #22799.

Caretaker note: Check in with the Domains team to make sure this passes their RTL tests on Guitar (feel free to message me to know who to contact)

@crisbeto crisbeto added P2 The issue is important to a large percentage of users, with a workaround target: patch This PR is targeted for the next patch release labels Jun 9, 2021
@crisbeto crisbeto requested review from andrewseguin and mmalerba June 9, 2021 05:49
@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jun 9, 2021
Copy link
Contributor

@mmalerba mmalerba left a comment

Choose a reason for hiding this comment

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

LGTM

@mmalerba mmalerba added the action: merge The PR is ready for merge by the caretaker label Jun 9, 2021
@andrewseguin andrewseguin added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Jun 9, 2021
@mmalerba
Copy link
Contributor

mmalerba commented Jun 9, 2021

Now that I think about it, should we hide the touch target element for lower densities? We don't need to guarantee accessible touch targets for lower densities, and I suspect most people would want to disable them anyways

@crisbeto
Copy link
Member Author

crisbeto commented Jun 9, 2021

I don't feel super strogly about it, but it seems like something that doesn't really hurt the experience on lower densities either. Also at what point would we hide the touch target?

@mmalerba
Copy link
Contributor

mmalerba commented Jun 9, 2021

I think it does hurt the experience, the touch target will over-hang quite a bit on lower densities, to the point where you can easily click the wrong button because its overlapping the neighboring button. (you could solve this by adding a bunch of margin, but that defeats the point of low density)

This is a reimplementation of angular#22846 which includes a fix for RTL layouts.

Adds some styles to ensure that the MDC-based button always has a touch target of at least 48px. Note that I decided against using MDC's built-in touch target class, because it would've added a margin on the button host node.

Fixes angular#22799.
@crisbeto crisbeto force-pushed the mdc-button-touch-target branch from 6b94299 to 6ba6c78 Compare June 9, 2021 19:08
@crisbeto
Copy link
Member Author

crisbeto commented Jun 9, 2021

I've pushed a change to remove the touch target on the two most dense configurations.

@wagnermaciel wagnermaciel merged commit 3284496 into angular:master Jun 14, 2021
wagnermaciel pushed a commit that referenced this pull request Jun 14, 2021
This is a reimplementation of #22846 which includes a fix for RTL layouts.

Adds some styles to ensure that the MDC-based button always has a touch target of at least 48px. Note that I decided against using MDC's built-in touch target class, because it would've added a margin on the button host node.

Fixes #22799.

(cherry picked from commit 3284496)
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jul 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note P2 The issue is important to a large percentage of users, with a workaround target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(material-experimental/mdc-button): should have 48px touch target at default density level

4 participants