feat: Food icons for the product page #6387

g123k · 2025-02-17T21:15:31Z

codecov-commenter · 2025-02-17T21:21:50Z

Codecov Report

Attention: Patch coverage is 13.00000% with 174 lines in your changes missing coverage. Please review.

Project coverage is 5.89%. Comparing base (4d9c7fc) to head (3d83514).
Report is 782 commits behind head on develop.

Files with missing lines	Patch %	Lines
.../smooth_app/lib/pages/product/attribute_icons.dart	0.00%	112 Missing ⚠️
...th_app/lib/pages/product/attribute_extensions.dart	0.00%	33 Missing ⚠️
...kages/smooth_app/lib/resources/app_food_icons.dart	50.00%	26 Missing ⚠️
...smooth_app/lib/helpers/attributes_card_helper.dart	0.00%	2 Missing ⚠️
...kages/smooth_app/lib/resources/app_icons_font.dart	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           develop   #6387      +/-   ##
==========================================
- Coverage     9.54%   5.89%   -3.66%     
==========================================
  Files          325     493     +168     
  Lines        16411   29446   +13035     
==========================================
+ Hits          1567    1736     +169     
- Misses       14844   27710   +12866

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

monsieurtanuki

Hi @g123k!

I'm a bit worried about repetitive code, and I'm sure some parts could be rewritten in a more compact and OOP way.

What about icons that are already in assets/cache? With this PR we have duplicates! Would it be possible to use your "new" icons only and to get rid of the old svg files? At least for the icons that are in both folders?

monsieurtanuki · 2025-02-18T08:02:53Z

packages/smooth_app/lib/resources/app_food_icons.dart

+  const FoodIcons.additives({
+    super.color,
+    super.size,
+    super.shadow,
+    super.semanticLabel,
+    super.key,
+  }) : super._(_FoodIconsFont.additives);


All that is very very verbose, for no added value.
What about one single constructor instead?

Suggested change

const FoodIcons.additives({

super.color,

super.size,

super.shadow,

super.semanticLabel,

super.key,

}) : super._(_FoodIconsFont.additives);

const FoodIcons({

super.color,

super.size,

super.shadow,

super.semanticLabel,

super.key,

required FoodIconFont icon,

}) : super._(icon);

I admit there's a lot of code, but it allows us to have const widgets directly.
Also, for other screens, we may need an "additive Widget" for example

monsieurtanuki · 2025-02-18T08:11:53Z

packages/smooth_app/lib/pages/product/attribute_icons.dart

+class _AttributeCeleryIcon extends AttributeIcon {
+  _AttributeCeleryIcon({
+    required super.backgroundColor,
+    required super.size,
+    super.foregroundColor,
+    super.semanticsLabel,
+  }) : super._(
+          icon: const FoodIcons.celery(),
+          iconSize: size! * 0.95,
+          offset: Offset(size * -0.1, 0.0),
+          clip: true,
+          padding: EdgeInsetsDirectional.only(top: size * 0.15),
+        );
+}


All that is quite verbose and redundant.
Would be easier to read with optional parameters stored at the FontIconFonts level, like:

sizeFactor: .95

clip: true

paddingFactor: Rect.fromLTRB(0, .15, 0, 0)

monsieurtanuki · 2025-02-18T08:12:52Z

packages/smooth_app/lib/pages/product/attribute_icons.dart

+      'additives' => _AttributeAdditivesIcon(
+          backgroundColor: backgroundColor,
+          foregroundColor: foregroundColor,
+          size: size,
+          semanticsLabel: semanticsLabel,
+        ),


All that redundant code also could be prevented, cf. later comments.

g123k · 2025-02-19T15:04:28Z

@monsieurtanuki Following your comments, all icons have:

an iconSizeFactor
a paddingFactor
an offsetFactor

monsieurtanuki

Hi @g123k!

Please have a look at my latest comments.

Currently what bothers me more than repetitive code is that you have repetitive icons too: what do you do with the icons currently in assets/cache?

packages/smooth_app/lib/pages/product/attribute_icons.dart

monsieurtanuki · 2025-02-19T16:16:21Z

packages/smooth_app/lib/pages/product/attribute_icons.dart

+              ? EdgeInsetsDirectional.only(
+                  start: paddingFactor!.start * size,
+                  end: paddingFactor!.end * size,
+                  top: paddingFactor!.top * size,
+                  bottom: paddingFactor!.bottom * size,
+                )


Just checking: does it mean that the display will be the same for LTR and RTL?

Mmm I don't really understand your point.
The idea of **Directional objects is to manage LTR and RTL automatically, by flipping left & right

My point here is that for some reason, you sometimes decide to move the icon slightly to the left or to the right, so that it looks better.
That would mean that you say start instead of left, the icon would be moved in the opposite direction, and would look worse.

monsieurtanuki · 2025-02-19T16:22:45Z

packages/smooth_app/lib/pages/product/attribute_icons.dart

+class _AttributeEggsIcon extends AttributeIcon {
+  const _AttributeEggsIcon({
+    required super.backgroundColor,
+    required super.size,
+    super.foregroundColor,
+    super.semanticsLabel,
+  }) : super._(
+          icon: const FoodIcons.eggs(),
+          iconSizeFactor: 0.65,
+          paddingFactor: const EdgeInsetsDirectional.only(top: 0.01),
+        );
+}


Suggested change

class _AttributeEggsIcon extends AttributeIcon {

const _AttributeEggsIcon({

required super.backgroundColor,

required super.size,

super.foregroundColor,

super.semanticsLabel,

}) : super._(

icon: const FoodIcons.eggs(),

iconSizeFactor: 0.65,

paddingFactor: const EdgeInsetsDirectional.only(top: 0.01),

);

}

class _AttributeFoodIcon extends AttributeIcon {

const _AttributeFoodIcon({

required super.backgroundColor,

required super.size,

super.foregroundColor,

super.semanticsLabel,

final FoodIcons icon,

}) : super._(

icon: icon,

iconSizeFactor: icon.iconSizeFactor,

paddingFactor: icon.paddingFactor,

clip: icon.clip,

offsetFactor: icon.offsetFactor,

angle: icon.angle,

);

}

Just checking: would that work? Then you wouldn't need all the classes.

Your code is correct.
If we want better perf with Flutter, it's always better to have dedicated widgets

I don't believe those tiny perf improvements have a significant impact on the overall perf of Smoothie, and I would prefer a more maintainable code.
Your code does work, though.

teolemon · 2025-02-19T19:04:58Z

@monsieurtanuki it's a case where we agreed with @stephanegigandet to move faster than the server for icon replacement.

monsieurtanuki · 2025-02-19T19:58:40Z

@monsieurtanuki it's a case where we agreed with @stephanegigandet to move faster than the server for icon replacement.

@teolemon Nothing wrong with that. But we already have tons of "cached" svg files with similar design and use-cases.
If we're not ready now to get rid of the old svg files, at least we could add a TODO about replacing them with the new font icons.

teolemon

A great visual improvement. Next step is having those in the config screen as well, as it gets revamped.

Co-authored-by: monsieurtanuki <[email protected]>

g123k · 2025-03-03T15:43:15Z

@monsieurtanuki it's a case where we agreed with @stephanegigandet to move faster than the server for icon replacement.

@teolemon Nothing wrong with that. But we already have tons of "cached" svg files with similar design and use-cases. If we're not ready now to get rid of the old svg files, at least we could add a TODO about replacing them with the new font icons.

For now, we keep them just in case.
But once the server will use the same UI as us, they will be removed from the app.

…dart" This reverts commit aaa1efc.

teolemon · 2025-03-03T17:29:26Z

Can i merge @g123k ?

monsieurtanuki · 2025-03-04T08:07:34Z

@monsieurtanuki it's a case where we agreed with @stephanegigandet to move faster than the server for icon replacement.

@teolemon Nothing wrong with that. But we already have tons of "cached" svg files with similar design and use-cases. If we're not ready now to get rid of the old svg files, at least we could add a TODO about replacing them with the new font icons.

For now, we keep them just in case. But once the server will use the same UI as us, they will be removed from the app.

So meanwhile either:

it's OK to have different icons in Smoothie for the same purpose - e.g. "may be vegetarian" - one from the server and one from this PR
it's OK to keep similar icons with approximately the same name and the very same purpose

Food icons for the product page

aa5e850

github-actions bot assigned g123k Feb 17, 2025

github-actions bot added 🥫 Product page dependencies labels Feb 17, 2025

Lint error

7e1a0e1

monsieurtanuki reviewed Feb 18, 2025

View reviewed changes

factor-like attributes

020350c

monsieurtanuki reviewed Feb 19, 2025

View reviewed changes

teolemon approved these changes Feb 20, 2025

View reviewed changes

Update packages/smooth_app/lib/pages/product/attribute_icons.dart

aaa1efc

Co-authored-by: monsieurtanuki <[email protected]>

g123k and others added 2 commits March 3, 2025 16:43

Merge branch 'develop' into food_icons

ec4e0e9

Revert "Update packages/smooth_app/lib/pages/product/attribute_icons.…

3d83514

…dart" This reverts commit aaa1efc.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Food icons for the product page #6387

feat: Food icons for the product page #6387

g123k commented Feb 17, 2025

codecov-commenter commented Feb 17, 2025 •

edited

Loading

monsieurtanuki left a comment

monsieurtanuki Feb 18, 2025

g123k Feb 18, 2025 •

edited

Loading

monsieurtanuki Feb 18, 2025

monsieurtanuki Feb 18, 2025

g123k commented Feb 19, 2025

monsieurtanuki left a comment

monsieurtanuki Feb 19, 2025

g123k Mar 3, 2025

monsieurtanuki Mar 4, 2025

monsieurtanuki Feb 19, 2025

g123k Mar 3, 2025

monsieurtanuki Mar 4, 2025

teolemon commented Feb 19, 2025

monsieurtanuki commented Feb 19, 2025

teolemon left a comment

g123k commented Mar 3, 2025

teolemon commented Mar 3, 2025

monsieurtanuki commented Mar 4, 2025

feat: Food icons for the product page #6387

Are you sure you want to change the base?

feat: Food icons for the product page #6387

Conversation

g123k commented Feb 17, 2025

codecov-commenter commented Feb 17, 2025 • edited Loading

Codecov Report

monsieurtanuki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

g123k Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

g123k commented Feb 19, 2025

monsieurtanuki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teolemon commented Feb 19, 2025

monsieurtanuki commented Feb 19, 2025

teolemon left a comment

Choose a reason for hiding this comment

g123k commented Mar 3, 2025

teolemon commented Mar 3, 2025

monsieurtanuki commented Mar 4, 2025

codecov-commenter commented Feb 17, 2025 •

edited

Loading

g123k Feb 18, 2025 •

edited

Loading