-
Notifications
You must be signed in to change notification settings - Fork 477
Conversation
hm for some reason this call is causing those |
b6ca245
to
38aa2fb
Compare
Codecov Report
@@ Coverage Diff @@
## master #2087 +/- ##
==========================================
+ Coverage 36.19% 44.90% +8.71%
==========================================
Files 153 122 -31
Lines 7047 5010 -2037
Branches 1640 1193 -447
==========================================
- Hits 2550 2249 -301
+ Misses 3279 1943 -1336
+ Partials 1218 818 -400 |
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.
- Looking good - except for a few missing
await
s - I don't have much context about why we need this. (I've read the PR description and the linked issue.)
- Because of that, I think I'm going to resign from approving/disapproving this PR. Maybe @brentvatne or @bbarthec could take a look at this?
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.
Left a few suggestions, but overall it looks good 💪 Good job on that! 🥳
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've misclicked approve
instead of request changes
😞
Co-authored-by: Dominik Sokal <dominik.sokal@swmansion.com> Co-authored-by: Bartłomiej Bukowski <bartlomiej.bukowski@swmansion.com>
5b88868
to
eff1748
Compare
packages/image-utils/src/Image.ts
Outdated
@@ -35,6 +35,9 @@ async function resizeAsync(imageOptions: ImageOptions): Promise<Buffer> { | |||
if (imageOptions.removeTransparency) { | |||
jimp.colorType(2); | |||
} | |||
if (imageOptions.circle) { | |||
jimp.circle(); |
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.
would like to do something similar here to how we're doing this same operation with sharp
, that way this could be a setting to round the corners instead of just cut out a circle, but was running into some mimeType issues trying to create a Sharp
image. but can look into that soon
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.
perhaps a borderRadius property would scale better
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.
changed it to borderRadius: number
. Right now if using Jimp any number will just make the image a circle though
Why?
This would automate setting the android icon and adaptive icons (Android 8+) on eject, and would enable developers to change their app icons much more easily when using the bare workflow. Plus, we could use the same code when building shell apps.
Testing
Run
yarn test
. CodeCov report should go up even more if we replace the current android icons code with calls toAndroidConfig.setIconAsync
😉Additional info
This should replace the current android icons code, and would fix expo/expo#7374 (if we remove the old AndroidIcons code and replace with a call to
Config.setIconAsync
)