Conversation
nitzanj
left a comment
nitzanj
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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;
}
3 participants
No description provided.