-
Notifications
You must be signed in to change notification settings - Fork 60
Glide Integration #111
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
Glide Integration #111
Conversation
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.
On top of the changes please add a glide integration test - get cloudinary to fetch a cloudinary image with public id and transformation and validate that the url it's trying to fetch matches the expected result.
|
||
private String publicId; | ||
private Transformation transformation; | ||
private ResponsiveUrl.Preset responsive; |
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 should be ResponsiveUrl
and not the preset, so the developer can have more flexibility. The constructor and getter should be updated accordingly.
The builder should have an overload to directly send in a preset for easier default usage (see below).
|
||
static final Option<Transformation> TRANSFORMATION = | ||
Option.memory("com.cloudinary.android.glide_cloudinary.CloudinaryRequestModelLoader.Transformation"); | ||
static final Option<ResponsiveUrl.Preset> RESPONSIVE = |
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.
Since we change it above obviously this too needs to become a ResponsiveUrl
responsive = options.get(RESPONSIVE); | ||
} | ||
if (responsive != null) { | ||
url = MediaManager.get().responsiveUrl(responsive).buildUrl(url, width, height); |
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.
Here's you'll have a ResponsiveUrl
already so you call buildUrl
directly (no need to go through MediaManager
.
|
||
private final String publicId; | ||
private Transformation transformation; | ||
private ResponsiveUrl.Preset responsive; |
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 build field should match the request itself so a ResponsiveUrl. But the builder should have two overloads to set it: One that accepts a preset (like the current one). And another that accepts a
responsiveUrl` to match the field.
* Set a responsive preset to be used when generating the resource. | ||
*/ | ||
public Builder responsive(ResponsiveUrl.Preset responsive) { | ||
this.responsive = responsive; |
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.
Since the fields will become ResponsiveUrl
but the parameter is a preset (in this overload), the setter will change a bit:
public Builder responsive(ResponsiveUrl.Preset preset) {
this.responsiveUrl = MediaManager.get().responsiveUrl(preset);
return this;
}
No description provided.