-
Notifications
You must be signed in to change notification settings - Fork 3.1k
[MaterialButton] Added methods to apply horizontal insets #1790
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
base: master
Are you sure you want to change the base?
[MaterialButton] Added methods to apply horizontal insets #1790
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.
Hey @gabrielemariotti,
Thanks for the PR! Added a few comments. Let me know what you think.
setHorizontalInsets(insetLeft, newInsetRight); | ||
} | ||
|
||
private void setHorizontalInsets(@Dimension int newInsetLeft, @Dimension int newInsetRight) { |
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.
What do you think about merging this method with setVerticalInsets
into something like setInsets(int, int, int, int)
or updateInsets(int, int, int, int)
? They're almost exactly the same, it might be nice to consolidate logic.
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.
Ok I will merge the 2 methods.
updateBackground(); | ||
} | ||
// Set the stored padding values | ||
ViewCompat.setPaddingRelative( |
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.
If we're using setPaddingRelative
here, don't we want to expose setInsetStart
and setInsetEnd
since internally setPaddingRelative
will swap the left/right values depending on the layout direction?
If we want to support setting left
and right
values, we'd need to change this to ViewCompat.setPadding()
, if I understand correctly.
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.
@hunterstich In the current code in the loadFromAttributes
you are using setPaddingRelative
adding paddingStart + insetLeft
. Let me know what you prefer. Maybe the best solution is to use insetLeft
, paddingLeft
and ViewCompat.setPadding()
everywhere.
material-components-android/lib/java/com/google/android/material/button/MaterialButtonHelper.java
Lines 128 to 133 in 5e3ccdc
ViewCompat.setPaddingRelative( | |
materialButton, | |
paddingStart + insetLeft, | |
paddingTop + insetTop, | |
paddingEnd + insetRight, | |
paddingBottom + insetBottom); |
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.
Oh good catch
I agree, it would be nice to correct/avoid the mix of paddingStart|End and left|right. Using paddingLeft|Right
+ insetLeft|Right
and ViewCompat.setPadding
everywhere sounds like a good solution to me. Off the top of my head, I can't think of any behavior that this might break, but once you update MaterialButtonHelper
and I pull it in and run all our test, we can double check.
In the future (or now if you want...), we can add in support for insetStart|End
and ViewCompat.setPaddingRelative
.
It closes #1770
This PR adds methods to support setting horizontal insets programatically.