-
-
Notifications
You must be signed in to change notification settings - Fork 287
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 NutriScore V2 into cache + semantics #5264
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5264 +/- ##
==========================================
- Coverage 9.54% 8.59% -0.96%
==========================================
Files 325 329 +4
Lines 16411 16768 +357
==========================================
- Hits 1567 1441 -126
- Misses 14844 15327 +483 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @g123k, LGTM!
Feel free to ignore my minor comments.
if (fileName == 'nutriscore-a.svg') { | ||
return localizations.nutriscore_a; | ||
} else if (fileName == 'nutriscore-b.svg') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sure a switch
syntax would be much easier to read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, with a return switch
statement, it's mandatory to have an else
case (didn't know that before trying after your suggestion).
I can "switch" to the old switch (with cases
), but in terms of lines, it will be similar
return localizations.nutriscore_not_applicable_new_formula; | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a matter of personal taste: as you return
you don't need an else
. Easier to read when there are no useless blocks.
2 => localizations.nutriscore_c, | ||
3 => localizations.nutriscore_d, | ||
4 => localizations.nutriscore_e, | ||
_ => localizations.nutriscore_unknown, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Not applicable" is not there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, for now, the animation lacks this state.
But I will integrate it with V2
Hi everyone,
Now that the Nutri-score V2 is mainly deployed, all SVG are put in cache.