Skip to content
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

Properly crop images in avatars API #3087

Merged
merged 6 commits into from Apr 26, 2022
Merged

Conversation

Meldiron
Copy link
Contributor

@Meldiron Meldiron commented Apr 12, 2022

What does this PR do?

Solves issue when resizing was not done properly if only one parameter was provided.

  • Fix Initials
  • Fix Flags
  • Fix Credit Cards
  • Fix Browsers
  • Fix URL Image

Not only we support proper resizing now, we also return full resolution unless asked to downscale.

Test Plan

  • Manual QA

CleanShot 2022-04-12 at 13 36 30
CleanShot 2022-04-12 at 13 37 48
CleanShot 2022-04-12 at 13 37 31
CleanShot 2022-04-12 at 13 37 08

(Yes, I tested all affected endpoints)

Related PRs and Issues

#3082

Have you read the Contributing Guidelines on issues?

Yes


@eldadfux
Copy link
Member

Not sure about this.

  1. this is a breaking change
  2. default image will have 0x0 size
  3. If this is the case, should we make both params required?

Could this be solved by just mentioning that providing the value 0 will keep the original width or height?

@Meldiron
Copy link
Contributor Author

@eldadfux

  1. Kind-of. Same code will still work, but instead of getting default size of 100x100, you will get source image at maximum resolution
  2. Correct and that's fine, that means original resolutions will be provided

Documenting endpoints better would also be a valid solution, yes.

@eldadfux
Copy link
Member

I think I would prefer to force the 100x100 resolution for preview images, to make devs aware of this instead of loading images in full resolution by default where most of the time it's far from optimized.

@Meldiron Meldiron requested a review from eldadfux April 20, 2022 08:31
@Meldiron
Copy link
Contributor Author

@eldadfux PR updated to only address documentation changes. It also includes fox when QR code size 0x0 was allowed, resulting in a server error.

Copy link
Member

@eldadfux eldadfux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor comment.

docs/references/avatars/get-browser.md Outdated Show resolved Hide resolved
@eldadfux
Copy link
Member

Approved, once conflict is resolved we can merge.

@Meldiron Meldiron changed the base branch from master to 0.14.x April 26, 2022 10:31
@christyjacob4 christyjacob4 merged commit 55901ad into 0.14.x Apr 26, 2022
@stnguyen90 stnguyen90 deleted the fix-avatar-cropping branch February 14, 2023 01:14
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.

None yet

3 participants