Skip to content

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

Merged
merged 5 commits into from
Jul 6, 2020
Merged

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