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

feat: add banner support for communities #2687

Merged
merged 1 commit into from May 27, 2022

Conversation

stefandunca
Copy link
Contributor

Add banner image as a special "banner" IdentityImage beside "thumbnail" and "large"

Changes to images module

  • Refactor EncodeToBestSize as EncodeToLimits to accept arbitrary dimmensions to allow for custom size
  • Define DimensionLimits for banner not to exceed 450 KB

Closes status-desktop #5403

@status-github-bot
Copy link

Hey @stefandunca, and thank you so much for making your first pull request in status-go! ❤️ Please help us make your experience better by filling out this brief questionnaire https://goo.gl/forms/uWqNcVpVz7OIopXg2

@status-github-bot
Copy link

status-github-bot bot commented May 23, 2022

Pull Request Checklist

  • Have you updated the documentation, if impacted (e.g. docs.status.im)?
  • Have you tested changes with mobile?
  • Have you tested changes with desktop?

@status-im-auto
Copy link
Member

status-im-auto commented May 23, 2022

Jenkins Builds

Click to see older builds (12)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 43c9584 #1 2022-05-23 10:09:21 ~2 min linux 📦zip
✔️ 43c9584 #1 2022-05-23 10:11:23 ~4 min ios 📦zip
✔️ 43c9584 #1 2022-05-23 10:13:12 ~6 min android 📦aar
✔️ 2988bca #2 2022-05-24 17:00:59 ~2 min ios 📦zip
✔️ 2988bca #2 2022-05-24 17:30:24 ~31 min android 📦aar
✔️ 2988bca #2 2022-05-24 17:32:28 ~33 min linux 📦zip
✔️ e3cc5e2 #3 2022-05-25 08:55:03 ~1 min linux 📦zip
✔️ e3cc5e2 #3 2022-05-25 08:55:33 ~1 min ios 📦zip
✔️ e3cc5e2 #3 2022-05-25 08:56:50 ~3 min android 📦aar
✔️ dd02321 #4 2022-05-25 23:59:32 ~1 min linux 📦zip
✔️ dd02321 #4 2022-05-25 23:59:51 ~2 min ios 📦zip
✔️ dd02321 #4 2022-05-26 00:00:58 ~3 min android 📦aar
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 44125b2 #5 2022-05-26 10:57:37 ~1 min linux 📦zip
✔️ 44125b2 #5 2022-05-26 10:58:32 ~2 min ios 📦zip
✔️ 44125b2 #5 2022-05-26 10:59:06 ~3 min android 📦aar
✔️ 7aef812 #6 2022-05-26 11:13:40 ~1 min linux 📦zip
✔️ 7aef812 #6 2022-05-26 11:14:15 ~2 min ios 📦zip
✔️ 7aef812 #6 2022-05-26 11:17:27 ~5 min android 📦aar

Copy link
Member

@Samyoul Samyoul left a comment

Choose a reason for hiding this comment

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

Looks good. A few points nothing major. One thing that I don't see is any tests for the banner functionality, do you feel confident in adding these?

images/encode.go Show resolved Hide resolved
images/meta.go Outdated Show resolved Hide resolved
images/meta.go Outdated Show resolved Hide resolved
protocol/requests/cropped_image.go Outdated Show resolved Hide resolved
@stefandunca stefandunca force-pushed the banner-dev branch 2 times, most recently from 2988bca to e3cc5e2 Compare May 25, 2022 08:53
Copy link
Contributor Author

@stefandunca stefandunca left a comment

Choose a reason for hiding this comment

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

... I don't see is any tests for the banner functionality, do you feel confident in adding these?

@Samyoul, how about I add integration tests to check that banner image processing respects the imposed size limits? However, that means working with big images, which might make tests longer than a few milliseconds. Is this a problem?

Do you have anything else in mind for the test scope?

Also, I'm not familiar with image processing in go. Any hints on what should I use to generate input test images so that I don't pollute the test source code with big files?

images/meta.go Outdated Show resolved Hide resolved
@Samyoul
Copy link
Member

Samyoul commented May 26, 2022

Perhaps check the IdentityImage components of ToCommunityDescription()

@stefandunca
Copy link
Contributor Author

stefandunca commented May 26, 2022

@Samyoul, I did all the requested changes, so please go ahead and have another look.

Also, after some tests with different compression quality, base resolution, and image complexity and talking to @John-44 I raised the ideal image quality to 300KB, which is an edge case, to target maximum quality.

I also changed the behaviour for banners not to upscale to target quality but only shrink if necessary. We can handle the upscaling in the UIs and save some space/traffic.

Add banner image as a special `IdentityImage` beside "thumbnail" and "large"

Banner input cropped image processing

- Resize to keep in the limits of `BannerDim`
- Encode to match the file size limits define for banner
- Don't scale up. This can be done efficiently in the UI

Changes to `images` module

- Refactor `EncodeToBestSize` as `EncodeToLimits` to accept arbitrary dimensions
  and allow for custom size
- Define `DimensionLimits` for banner not to exceed 450 KB and a rough estimate
  for the ideal size
Copy link
Member

@Samyoul Samyoul left a comment

Choose a reason for hiding this comment

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

Great, looks good

@stefandunca stefandunca merged commit 63e58ba into status-im:develop May 27, 2022
Client testing status-go automation moved this from Contributor to DONE May 27, 2022
@stefandunca stefandunca deleted the banner-dev branch May 30, 2022 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Add banner support to backend (status-go)
4 participants