-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add true support for vector drawables #814
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
Conversation
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
|
|
||
| <style name="FirebaseUI.Button.AccountChooser.EmailButton"> | ||
| <item name="android:drawableLeft">@drawable/fui_ic_mail_white_24dp</item> | ||
| <item name="drawableStartCompat">@drawable/fui_ic_mail_white_24dp</item> |
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.
Pre-L this will crash with a res not found exception if we don't manually get the drawable with AppCompatResources.getDrawable.
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.
So is this the reason for the button subclass?
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.
Yup.
|
Oh nice! I'll take another look at this early next week (and your other PRs, I have been slacking on this repo). I remember when we went with vectors for Santa Tracker there were a bunch of gotchas and I just want to make sure they don't apply here (I think we're OK). |
|
@samtstern SGTM! (I also have another PR coming up 😊) |
|
Very cool to see this change! #vectorallthethings |
|
@samtstern Was this closed by accident or did you decide against it? |
|
@SUPERCILEX accident! Sorry haha. |
|
@samtstern 😄👍 |
# Conflicts: # auth/src/main/res/layout/fui_phone_progress_dialog.xml
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
app/src/main/res/values/config.xml
Outdated
| how obfuscated, makes it vulnerable to being stolen. | ||
| --> | ||
| <string name="twitter_consumer_secret" translatable="false">CHANGE-ME</string> | ||
| <string name="twitter_consumer_secret" translatable="false">di6UQRwDaxsRQ7RT5M5QcUgq19Dn4NY04QLMjaDhyxqmUy92Pe</string> |
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.
This seems like a mistake haha
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 lol, yep.
|
|
||
| import com.firebase.ui.auth.R; | ||
|
|
||
| public class SupportVectorDrawablesButton extends AppCompatButton { |
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.
This class seems to have some cool but non-obvious compatibility tricks. Can you add comments (class level and in the one big method) explaining what's going on?
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
|
LGTM! |
@samtstern I didn't know this at the time, but we were never actually getting the true benefits of vector drawables because the build tools would just convert them to pngs at the end of the day. I went through the pains of figuring out how to enable true vector drawable support for my app and I'm bringing this knowledge over to FUI. Yay! 😄
Turns out we actually save quite a few kbs considering how few drawables we have: