-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Refactor: Remove ImageLoader usages from Blaze #23891
Conversation
Generated by 🚫 Danger |
📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
|
@@ -28,7 +28,7 @@ final class ImageRequest { | |||
} | |||
|
|||
struct ImageRequestOptions { | |||
/// Resize the thumbnail to the given size. By default, `nil`. | |||
/// Resize the thumbnail to the given size (in pixels). By default, `nil`. | |||
var size: CGSize? |
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 feel like I have commented about this before... I apologize if you have already answered this question... 😅
I think it's good to express "the given size (in pixels)" in code. Like sizeInPixel: CGSize
, or creating a struct PixelSize { pixelWidth, pixelHeight }
type.
It's easy to miss documentation, but it's very hard to miss if you have to write "pixel" in code.
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.
Yes. I already attempted it once by adding an ImageSize
struct, but then realized the name was already taken – ugh! I added a note to try it again later 😄
PixelSize
sounds. I can introduce it in a separate PR. It could also have a convenience initializer that takes points and multiplies by the display scale.
5bc3c60
to
499c6e6
Compare
499c6e6
to
73f82b3
Compare
📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
|
This brings us one step closer to removing
AlamofireImage
and consolidating all the image management code inImageDownloader
andAsyncImageView
.My initial idea was to update the existing (deprecated)
ImageLoader
to useImageDownloader
, but after a quick search, I realized that there were only seven remaining places where it was used. So, I decided to removeImageLoader
usages instead.Changes:
AsyncImageView
Before / After
To test:
Regression Notes
Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
What automated tests I added (or what prevented me from doing so)
PR submission checklist:
RELEASE-NOTES.txt
if necessary.Testing checklist: