Supporting Material Library#124
Supporting Material Library#124Zoha131 wants to merge 7 commits intoairbnb:masterfrom Zoha131:material
Conversation
…ndroid app module
ngsilverman
left a comment
ngsilverman
left a comment
There was a problem hiding this comment.
Hi @Zoha131, thank you for your contribution and sorry it took me a while to review it. I think you're on the right path here but I've got some comments. I think this can be simplified in a number of ways. If you can work on it some more I can take a closer look and try to see how to best address the textAppearance issue.
| @@ -0,0 +1,119 @@ | |||
| <component name="ProjectCodeStyleConfiguration"> | |||
There was a problem hiding this comment.
Please don't commit this file.
| MOCKITO_VERSION = '2.23.0' | ||
| ROBOLECTRIC_VERSION = '3.8' | ||
| TESTING_COMPILE_VERSION = '0.15' | ||
| MATERIAL_VERSION = '1.1.0-rc01' |
There was a problem hiding this comment.
Could we use a stable version instead?
|
|
||
| private var mColors: ColorStateList? = null | ||
| private val defaultTextAppearance:Int = 16974317 //todo this might not be the default for every android version or phone | ||
| private var mTextAppearance = defaultTextAppearance |
There was a problem hiding this comment.
This project doesn't follow the m prefix convention. You can call these colors and textAppearance.
| } | ||
|
|
||
| @Attr(R2.styleable.Paris_MaterialButton_android_textColor) | ||
| fun setTextColor(colors: ColorStateList?) { |
There was a problem hiding this comment.
MaterialButton extends from TextView so all the attributes supported by TextViewProxy and ViewProxy should already work. They don't need to be added again here.
| /* | ||
| * Material component has default textAppearance. | ||
| * But if we do not pass any textAppearance then | ||
| * android default textAppearance is set. To keep the |
There was a problem hiding this comment.
If the style doesn't define textAppearance and the theme does then the theme value will be used. That's probably what you're observing and it is the correct behavior. I don't think we should be trying to override it here.
| if(mColors == null){ | ||
| val typedValue = TypedValue() | ||
| view.context.theme.resolveAttribute(R.attr.colorOnPrimary, typedValue, true) | ||
| view.setTextColor(typedValue.data) |
There was a problem hiding this comment.
How is using colorOnPrimary implemented in MaterialButton? This may be unnecessary.
|
|
||
| object ViewUtils { | ||
|
|
||
| fun parseTintMode(value: Int, defaultMode: PorterDuff.Mode): PorterDuff.Mode { |
There was a problem hiding this comment.
Reiterating a comment above: this is already implemented by ViewProxy and is not needed here.
| @@ -0,0 +1,27 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
| <resources> | |||
| <declare-styleable name="Paris_MaterialButton"> | |||
There was a problem hiding this comment.
This should only include the XML attributes that are specifically owned by the MaterialButton class (see https://developer.android.com/reference/com/google/android/material/button/MaterialButton), not its parents.
| @@ -0,0 +1,25 @@ | |||
| apply plugin: 'com.android.application' | |||
There was a problem hiding this comment.
I don't think we need a sample app specifically for material, you can use the existing sample app.
2 participants
Adding MaterialButtonProxy to support dynamic style for MaterialButton.
As stated in the issue: #91 I am submitting this PR for review.
The problems I am facing:
For TextColor and TextAppearance when the user does not pass any value then by default it does not pick the material theme color
When I tried to add style using the style resource then I get some unexpected behavior. It does not work as intended.
Code Structure I have used: