-
Notifications
You must be signed in to change notification settings - Fork 984
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
[mobile] switch to png icons #8400
Conversation
Pull Request Checklist
|
Jenkins BuildsClick to see older builds (25)
|
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 about removing svgs ?
(update icon-vec 1 assoc :width width :height height) | ||
icon-vec)) | ||
(throw (js/Error. (str "Unknown icon: " name))))]) | ||
[react/view |
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.
interesting why was it animated?
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.
no idea
@flexsurfer they are used on desktop; right now I'm trying to verify if there is actually any improvement |
98% of end-end tests have passed
Failed tests (1)Click to expand
Passed tests (48)Click to expand |
Teststed on slow device:
So with png the animation starts much earlier, with svg it is delayed for at least 3 frames (6 at 60fps), which is 100ms.
Again, it is close to 100ms difference. Most likely it will be similar number for each transition, as we usually have 4-5 icons on each screen. There is no big difference in the whole transition time, but the animation is delayed in case if svg is used. |
@errorists |
@rasom gotcha. Since these are not resolution independent anymore, can you tell me at which size (1x) we need the app logo? Is the largest one on the splash screen? |
thanks, exported one at 46x46 here Dropbox |
@rasom we should also use the 46 in all those instances since it's larger, downsizing is perfectly fine with PNGs. |
@errorists ok then, thanks |
@errorists it actually was 46 for "S" icon and 111 for the whole thing... |
@rasom 🤔 111 is a weird number to have but anyway this will be updated with new onboarding. Updated the resources in the dropbox link above, it's 111 now. |
Several issues after migration to PNG (some of them are iOS specific):
|
@rasom hey, this has me wondering, do we need to supply the Android specific image formats so |
@Serhy issues should be fixed now |
@rasom it's good, one last item though (sorry didn't notice on the previous build) |
@Serhy I just pushed fix |
100% of end-end tests have passed
Passed tests (47)Click to expand
|
@rasom, perfect! Thanks a lot! |
status: ready