Skip to content

Conversation

@baraka-gini
Copy link
Contributor

No description provided.

@asisayag2 asisayag2 requested a review from nitzanj April 13, 2020 20:25
@baraka-gini baraka-gini changed the title Add Glide Integration Glide Integration Apr 14, 2020
Copy link
Contributor

@nitzanj nitzanj left a 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;
Copy link
Contributor

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 =
Copy link
Contributor

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);
Copy link
Contributor

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;
Copy link
Contributor

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;
Copy link
Contributor

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;
}

@nitzanj nitzanj merged commit 6ced3a0 into cloudinary:master Jul 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants