feat: 5817 Product page: tab Data #5953
Conversation
AdrienMDevMobile
changed the title
|
@g123k could you tell me please, if squash on merge is allowed on this project ? |
Yes, that's what we do: |
|
…ign pour nutritions
|
@AdrienMDevMobile don't hesitate to ping people in |
|
@AdrienMDevMobile You still have issues with your PR: https://github.com/openfoodfacts/smooth-app/actions/runs/12238508540/job/34210431480?pr=5953 |
| appLocalizations.edit_product_form_item_stores_title, | ||
| }; | ||
|
|
||
| AppIcon toAppIcon() => switch (this) { |
There was a problem hiding this comment.
Just in case, return the more generic Widget here
| ? const Color.fromRGBO(228, 228, 228, 1.0) | ||
| : Colors.white; | ||
|
|
||
| return Container( |
There was a problem hiding this comment.
If you use just 1 element from a Container, please use the correct Widget directly.
Here it's a Padding.
| : Colors.white; | ||
|
|
||
| return Container( | ||
| margin: const EdgeInsets.only(left: 90.0), |
There was a problem hiding this comment.
Please avoid EdgeInsets and prefer the more generic EdgeInsetsDirectional
| controller: widget.controller, | ||
| physics: const ClampingScrollPhysics(), | ||
| shrinkWrap: true, | ||
| padding: const EdgeInsets.symmetric( |
There was a problem hiding this comment.
Same here + when you have semi-values like, please switch to a predefined constant (eg: MEDIUM_SPACE)
| child: ListView.separated( | ||
| controller: widget.controller, | ||
| physics: const ClampingScrollPhysics(), | ||
| shrinkWrap: true, |
There was a problem hiding this comment.
ShrinkWrap is your enemy. Why do you use it?
There was a problem hiding this comment.
What is wrong with ShrinkWrap ?
In my case, it is needed to not have a crash due to ListView inside the other list view.
| ], | ||
| ), | ||
| ), | ||
| Divider( |
There was a problem hiding this comment.
Why do you need a Divider of 0?
There was a problem hiding this comment.
the divider default value is not 0 but 16
|
|
||
| class ProductRawDataElementItem extends StatelessWidget { | ||
| const ProductRawDataElementItem(this.element, this.onSeeMoreTap, | ||
| {this.controller}); |
| children: [ | ||
| Text((element as ProductRawDataElementDoubleText).text2), | ||
| const SizedBox( | ||
| width: 29, |
There was a problem hiding this comment.
By convention, when we have double, we use 29.0
| { | ||
| final AppLocalizations appLocalizations = | ||
| AppLocalizations.of(context); | ||
| return GestureDetector( |
There was a problem hiding this comment.
We're not fans of GestureDetector. Could you use an InkWell instead?
| return Scaffold( | ||
| body: ListView.separated( | ||
| itemCount: productRawDatas.length, | ||
| separatorBuilder: (BuildContext context, _) => Divider( |
There was a problem hiding this comment.
Same here, why a Divider of 0?
There was a problem hiding this comment.
//remove default margin between elements
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #5953 +/- ##
==========================================
- Coverage 9.54% 6.50% -3.05%
==========================================
Files 325 441 +116
Lines 16411 24982 +8571
==========================================
+ Hits 1567 1624 +57
- Misses 14844 23358 +8514 ☔ View full report in Codecov by Sentry. |
What
Screenshot
Fixes bug(s)
Part of