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

fix: 5744 - cached donation_campaign fr.svg en.svg #5748

Closed
wants to merge 7 commits into from

Conversation

monsieurtanuki
Copy link
Contributor

What

Fixes bug(s)

Files

New file:

  • donation_campaign__fr.svg

Impacted files:

  • abstract_cache.dart: now we may cache svg files including their folder names
  • svg_cache.dart: potentially folder names for cached svg files

New file:
* `donation_campaign__fr.svg`

Impacted files:
* `abstract_cache.dart`: now we may cache svg files including their folder names
* `svg_cache.dart`: potentially folder names for cached svg files
@codecov-commenter
Copy link

codecov-commenter commented Oct 27, 2024

Codecov Report

Attention: Patch coverage is 0% with 16 lines in your changes missing coverage. Please review.

Project coverage is 7.07%. Comparing base (4d9c7fc) to head (031d10f).
Report is 434 commits behind head on develop.

Files with missing lines Patch % Lines
...smooth_app/lib/cards/category_cards/svg_cache.dart 0.00% 9 Missing ⚠️
...h_app/lib/cards/category_cards/abstract_cache.dart 0.00% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           develop   #5748      +/-   ##
==========================================
- Coverage     9.54%   7.07%   -2.48%     
==========================================
  Files          325     411      +86     
  Lines        16411   22246    +5835     
==========================================
+ Hits          1567    1574       +7     
- Misses       14844   20672    +5828     

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

Copy link
Member

@teolemon teolemon left a comment

Choose a reason for hiding this comment

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

@g123k @stephanegigandet @monsieurtanuki we could go for the full path of the image ?

@g123k
Copy link
Collaborator

g123k commented Oct 27, 2024

We don't need to cache this image and the same goes for en.svg.
This if for the donation campaign in the tagline. It will disappear in a few weeks.

@monsieurtanuki
Copy link
Contributor Author

we could go for the full path of the image ?

I don't think we can afford very long filenames in flutter asset folders, therefore I wouldn't recommend that (with a limited added value, too).

We don't need to cache this image and the same goes for en.svg. This if for the donation campaign in the tagline. It will disappear in a few weeks.

I wouldn't say we don't need to cache these images, as they're supposed to be displayed quickly and on the home page.
That said, we can create an issue at an appropriate time in order to say "get rid of those now useless files donation_campaign__fr.svg and donation_campaign__en.svg.

@monsieurtanuki monsieurtanuki changed the title fix: 5744 - cached donation_campaign/fr.svg fix: 5744 - cached donation_campaign fr.svg en.svg Oct 27, 2024
@g123k
Copy link
Collaborator

g123k commented Oct 27, 2024

we could go for the full path of the image ?

I don't think we can afford very long filenames in flutter asset folders, therefore I wouldn't recommend that (with a limited added value, too).

We don't need to cache this image and the same goes for en.svg. This if for the donation campaign in the tagline. It will disappear in a few weeks.

I wouldn't say we don't need to cache these images, as they're supposed to be displayed quickly and on the home page. That said, we can create an issue at an appropriate time in order to say "get rid of those now useless files donation_campaign__fr.svg and donation_campaign__en.svg.

Honestly, I would prefer to fix things correctly by displaying the news only if the image is available/loaded.

Hence, we don't embed unnecessary things (e.g.: why do we store the FR variant for non-French users?) and we never forget them indefinitely.

Minor fix:

  • Ensure assets have a better name

@monsieurtanuki
Copy link
Contributor Author

why do we store the FR variant for non-French users?

Oh, you mean we should get rid of cached localized nutriscore files too?
Screenshot_20241027_182117_Chrome

and we never forget them indefinitely.

I did mention an issue to get rid of them. I'm not sure that all the currently cached svg files are still relevant either anyway.

@teolemon teolemon added asset cache cached assets (SVG…) for the knowledge panels 🎁 Donations labels Nov 5, 2024
@monsieurtanuki
Copy link
Contributor Author

Not very relevant anymore as donation campaign svg filenames are less ambiguous now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
asset cache cached assets (SVG…) for the knowledge panels 🎁 Donations
Development

Successfully merging this pull request may close these issues.

New svg files to cache, but without an explicit enough name
4 participants