-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
fix(material-experimental/mdc-button): set proper touch target #22931
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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 |
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? |
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.
6b94299
to
6ba6c78
Compare
I've pushed a change to remove the touch target on the two most dense configurations. |
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)
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
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)