8000 MBL-2116 images on rewards cleanup by ycheng-kickstarter · Pull Request #2261 · kickstarter/android-oss · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

MBL-2116 images on rewards cleanup #2261

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 18 commits into from
Mar 27, 2025
Merged

Conversation

ycheng-kickstarter
Copy link
Contributor
@ycheng-kickstarter ycheng-kickstarter commented Mar 19, 2025

📲 What

Will wait until after code freeze to merge as it is not required for the latest release and I'd like some time for QA

Cleaning up Image on Rewards with some nice to haves:

- Refactor AsyncImage to Coil’s SubcomposeAsyncImage to add loading states to images
Due to performance concerns mentioned in SubcomposeAsyncImage's documentation, will continue to use AsyncImage instead. Confirmed with Alison that using a gray placeholder instead of a loading indicator is fine for images. FYI @Arkariang

NOTE: This API uses subcomposition, which is slow. Avoid using this composable in places that need high performance (e.g. LazyRow/LazyColumn).

  • Create a compose version of KSImage that can handle these internal configurations
  • Make image size dynamic in graphql query
  • Remove unused adapter/item files related to rewards/addons:
    - RewardCardAdapter
    - item_reward
    - RewardViewHolder

🤔 Why

🛠 How

👀 See

Check Rewards Carousel and Add On screen to see the new loading state (a gray linear progress indicator)
No user facing changes

📋 QA

Story 📖

https://kickstarter.atlassian.net/browse/MBL-2116

@codecov-commenter
Copy link
codecov-commenter commented Mar 19, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 67.01%. Comparing base (6b2bd8f) to head (eb6e8f6).

Files with missing lines Patch % Lines
.../activities/compose/projectpage/AddOnsContainer.kt 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #2261      +/-   ##
============================================
+ Coverage     66.98%   67.01%   +0.02%     
+ Complexity     2246     2245       -1     
============================================
  Files           363      363              
  Lines         24815    24805      -10     
  Branches       3654     3654              
============================================
  Hits          16622    16622              
+ Misses         6298     6288      -10     
  Partials       1895     1895              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ycheng-kickstarter ycheng-kickstarter marked this pull request as ready for review March 24, 2025 14:09
@ycheng-kickstarter ycheng-kickstarter marked this pull request as draft March 26, 2025 14:33
@ycheng-kickstarter
Copy link
Contributor Author

Moving this back to draft due to concerns about SubcomposeAsyncImage performance.
Per SubcomposeAsyncImage documentation:

NOTE: This API uses subcomposition, which is slow. Avoid using this composable in places that need high performance (e.g. LazyRow/LazyColumn).

@ycheng-kickstarter ycheng-kickstarter marked this pull request as ready for review March 26, 2025 14:47
Copy link
Contributor
@Arkariang Arkariang left a comment

Choose a reason for hiding this comment

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

👍 lgtm, thanks for the clean up/investigation yun!

@ycheng-kickstarter ycheng-kickstarter merged commit 982dfab into master Mar 27, 2025
3 checks passed
@ycheng-kickstarter ycheng-kickstarter deleted the MBL-2116-images-cleanup branch March 27, 2025 19:32
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.

4 participants
0