-
Notifications
You must be signed in to change notification settings - Fork 13
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
Add android 36dp icons #38
Conversation
android:viewportHeight="36"> | ||
<path | ||
android:fillColor="#0C0C0D" | ||
android:fillAlpha=".8" |
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.
@aminalhazwani The fillAlpha in the 24dp assets was creating issues matching the colors specified in the specs: is there any reason it's included here? fwiw, we removed the fillAlpha
from the 24dp assets in components.
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.
Relatedly, any changes we make in components should probably be reflected here, upstream, right? Do you have any ideas on how we can make sure that happens?
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.
The fillAlpha in the 24dp assets was creating issues matching the colors specified in the specs: is there any reason it's included here?
@mcomella I think is there because of how the svg2vectordrawable
package works, I mentioned something similar here #35 (comment)
Relatedly, any changes we make in components should probably be reflected here, upstream, right? Do you have any ideas on how we can make sure that happens?
Maybe task cluster? So that every time an update is push a PR/Issue is open?
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.
@mcomella specifically to connect you can set the fillColor
to Grey 10 with no fillAlpha
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.
@mcomella specifically to connect you can set the
fillColor
to Grey 10 with nofillAlpha
I removed fillAlpha
and tinted it in code (rather than changing the assets).
@mcomella I think is there because of how the
svg2vectordrawable
package works, I mentioned something similar here #35 (comment)
Is the fillAlpha
desirable? Assuming you execute this on the command line, I could quickly write you a wrapper script that would remove fillAlpha
from the output (using sed
).
Relatedly, any changes we make in components should probably be reflected here, upstream, right? Do you have any ideas on how we can make sure that happens?
Maybe task cluster? So that every time an update is push a PR/Issue is open?
Filed mozilla-mobile/android-components#817 to come up with a solution.
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.
Is the fillAlpha desirable? Assuming you execute this on the command line, I could quickly write you a wrapper script that would remove fillAlpha from the output (using sed).
@mcomella we have a defined set of fill colors that we currently support, this selection goes across platforms, so not Android-only
- Grey 10
- Grey 10 a80
- Grey 10 a60
- Grey 90 a80
- Grey 90 a60
- Blue 40
- Blue 50
- Green 60
- Red 60
- White 100
Would be interesting to understand how can we enable engineering to pick only from those colors so that they don't have to manually modify assets 🙂
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.
I don't think there's an easy way to do this. I filed: mozilla-mobile/android-components#935
Traditionally, we do separate color from asset:
android:src="@drawable/components_asset
android:tint="@color/photon_grey10"
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.
Looks awesome to me! Let's ship it, and follow up with further changes as necessary. 🙂
For Amazon products we need larger Android icons, this PR adds the minimum necessary to move the project forward and open a PR against the
android-components
repository. We will add the missing 36dp icons later on.