-
-
Notifications
You must be signed in to change notification settings - Fork 291
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: 5 bugfixes (icons color, better network mgmt / Japanese…) #6127
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6127 +/- ##
==========================================
- Coverage 9.54% 6.38% -3.17%
==========================================
Files 325 448 +123
Lines 16411 25502 +9091
==========================================
+ Hits 1567 1628 +61
- Misses 14844 23874 +9030 ☔ 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!
Please have a look at my comments.
Beyond them:
- several circular progress indicators at the same time is a good idea to provoke headaches - especially if you're already cursed with low connectivity. A less aggressive OFF specific "wait indicator" may be more appropriate
- I still think it would be relevant to cache the home page "support us" logo
frameBuilder: (_, Widget child, int? frame, ____) { | ||
if (frame == null) { | ||
return const Center( | ||
child: CircularProgressIndicator(), |
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.
child: CircularProgressIndicator(), | |
child: CircularProgressIndicator.adaptive(), |
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.
Here I'm deliberately forcing the non-adaptive variant, because on iOS, the loader is very small.
packages/smooth_app/lib/cards/product_cards/smooth_product_image.dart
Outdated
Show resolved
Hide resolved
: null, | ||
), | ||
child: const Center( | ||
child: CircularProgressIndicator(), |
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.
child: CircularProgressIndicator(), | |
child: CircularProgressIndicator.adaptive(), |
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.
Same here.
I'm deliberately forcing the non-adaptive variant, because on iOS, the loader is very small.
@@ -69,7 +69,7 @@ class SmoothTheme { | |||
? Colors.white | |||
: myColorScheme.onPrimary, | |||
), | |||
iconColor: WidgetStateProperty.all<Color>(Colors.white), | |||
iconColor: WidgetStateProperty.all<Color>(myColorScheme.onPrimary), |
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.
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.
True, but the issue is when we don't specify the color.
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.
@raphodn some context we could expose to users a out those colors ?
…ge.dart Co-authored-by: monsieurtanuki <[email protected]>
Does it work with orange or green price labels?
________________________________
De : Edouard Marquez ***@***.***>
Envoyé : dimanche 5 janvier 2025 20:24
À : openfoodfacts/smooth-app ***@***.***>
Cc : monsieurtanuki ***@***.***>; Comment ***@***.***>
Objet : Re: [openfoodfacts/smooth-app] fix: 5 bugfixes (icons color, better network mgmt / Japanese…) (PR #6127)
@g123k commented on this pull request.
________________________________
In packages/smooth_app/lib/themes/smooth_theme.dart<#6127 (comment)>:
@@ -69,7 +69,7 @@ class SmoothTheme {
? Colors.white
: myColorScheme.onPrimary,
),
- iconColor: WidgetStateProperty.all<Color>(Colors.white),
+ iconColor: WidgetStateProperty.all<Color>(myColorScheme.onPrimary),
True, but the issue is when we don't specify the color.
—
Reply to this email directly, view it on GitHub<#6127 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACYKI3Y5QRULZT45LH4XC2T2JGBH5AVCNFSM6AAAAABUTLPWZ6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKMZQHE2TENJTGQ>.
You are receiving this because you commented.Message ID: ***@***.***>
|
Ok, this is fixed for prices buttons, where we say icon color = foreground color |
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.
The bugs being fixed, let's go!
Hi everyone!
Here are 4 fixes:
The icon color in dark mode is now OK (my fault) Dark mode issues are back #6122
When the image in the tagline fails to load, the image container is hidden in all cases (non-vectorial + svg)
Product picture: force a loader when the image is loading
Product gallery: force a loader when the image is loading
A table width can't have a width of 0 Weird table layout in Japanese #6125