feat: 3332 - crop page title now reflects image field #3366

monsieurtanuki · 2022-11-27T10:51:16Z

Impacted files:

confirm_and_upload_picture.dart: minor refactoring
crop_helper.dart: added a pageTitle parameter
edit_ingredients_page.dart: minor refactoring
image_crop_page.dart: added imageField parameters, so that we can compute the correct label on crop page
image_upload_card.dart: now we explicitly use getProductImageButtonText as label
new_crop_page.dart: added a page title parameter
product_cards_helper.dart: new label method getImagePageTitle; minor refactoring
product_image_carousel.dart: minor refactoring
product_image_data.dart: removed label fields - now we have more flexibility, computing labels from imageField with product_cards_helper.dart methods
product_image_gallery_view.dart: minor refactoring
product_image_swipeable_view.dart: now we explicitly use getImagePageTitle as label
product_image_viewer.dart: minor refactoring
smooth_images_sliver_list.dart: now we explicitly use getProductImageTitle as label
smooth_product_image.dart: minor refactoring

What

Very first step of Simplify adding photos by removing 2 of the 4 steps #3332: the crop page title now reflects the image field (instead of a mere "Edit image")
That was a good opportunity to refactor actually, by removing "labels" from ProductImageData that were not really useful

Screenshot

Part of

Simplify adding photos by removing 2 of the 4 steps #3332

Impacted files: * `confirm_and_upload_picture.dart`: minor refactoring * `crop_helper.dart`: added a `pageTitle` parameter * `edit_ingredients_page.dart`: minor refactoring * `image_crop_page.dart`: added `imageField` parameters, so that we can compute the correct label on crop page * `image_upload_card.dart`: now we explicitly use `getProductImageButtonText` as label * `new_crop_page.dart`: added a page title parameter * `product_cards_helper.dart`: new label method `getImagePageTitle`; minor refactoring * `product_image_carousel.dart`: minor refactoring * `product_image_data.dart`: removed label fields - now we have more flexibility, computing labels from `imageField` with `product_cards_helper.dart` methods * `product_image_gallery_view.dart`: minor refactoring * `product_image_swipeable_view.dart`: now we explicitly use `getImagePageTitle` as label * `product_image_viewer.dart`: minor refactoring * `smooth_images_sliver_list.dart`: now we explicitly use `getProductImageTitle` as label * `smooth_product_image.dart`: minor refactoring

monsieurtanuki · 2022-11-27T10:57:35Z

Possible variations:

"edit" before	"edit" after

codecov-commenter · 2022-11-27T10:58:56Z

Codecov Report

Merging #3366 (f430035) into develop (5fe74ca) will increase coverage by 0.00%.
The diff coverage is 0.00%.

❗ Current head f430035 differs from pull request most recent head 1977488. Consider uploading reports for the commit 1977488 to get more accurate results

@@           Coverage Diff            @@
##           develop    #3366   +/-   ##
========================================
  Coverage    10.41%   10.41%           
========================================
  Files          258      258           
  Lines        12494    12492    -2     
========================================
  Hits          1301     1301           
+ Misses       11193    11191    -2

Impacted Files	Coverage Δ
...th_app/lib/cards/data_cards/image_upload_card.dart	`0.00% <0.00%> (ø)`
...ib/cards/product_cards/product_image_carousel.dart	`0.00% <0.00%> (ø)`
...smooth_app/lib/data_models/product_image_data.dart	`0.00% <ø> (ø)`
..._lib/widgets/images/smooth_images_sliver_list.dart	`0.00% <0.00%> (ø)`
.../lib/generic_lib/widgets/smooth_product_image.dart	`0.00% <0.00%> (ø)`
...s/smooth_app/lib/helpers/product_cards_helper.dart	`0.00% <0.00%> (ø)`
packages/smooth_app/lib/pages/crop_helper.dart	`0.00% <0.00%> (ø)`
packages/smooth_app/lib/pages/image_crop_page.dart	`1.35% <0.00%> (-0.04%)`	⬇️
.../lib/pages/product/confirm_and_upload_picture.dart	`0.00% <0.00%> (ø)`
...h_app/lib/pages/product/edit_ingredients_page.dart	`0.00% <0.00%> (ø)`
... and 4 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

monsieurtanuki · 2022-11-27T11:03:28Z

@omkarChend1kar ping

omkarChend1kar · 2022-11-28T15:09:52Z

@omkarChend1kar ping

Hii @monsieurtanuki ,I didn't get you.

AshAman999 · 2022-11-28T15:17:17Z

@omkarChend1kar ping

Hii @monsieurtanuki ,I didn't get you.

I think it's time you review this PR!!

omkarChend1kar · 2022-11-28T17:07:23Z

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

@@ -65,11 +65,7 @@ class _ProductImageSwipeableViewState extends State<ProductImageSwipeableView> {
    context.watch<LocalDatabase>();
    _product = _localDatabase.upToDate.getLocalUpToDate(_initialProduct);
    final List<ProductImageData> allProductImagesData =
-        getProductMainImagesData(
-      _product,
-      appLocalizations,


Everything looks good!!

Though ,If instance of AppLocalizations is not required anymore to get the list of ProductImagesData, Then it's declaration can be removed, Since not required anywhere else on this page.

monsieurtanuki · 2022-11-28T17:14:52Z

Thank you @omkarChend1kar for your review!
Actually you're wrong, we still use it there: getImagePageTitle(appLocalizations, _imageDataList[index].imageField)
There would have been a warning if useless anyway.

@M123-dev @AshAman999 May you approve too? Not that I don't trust @omkarChend1kar, but for some reason I'm not able to merge even with one approval. Not enough seniority on this project maybe...

omkarChend1kar · 2022-11-28T17:21:37Z

Thank you @omkarChend1kar for your review! Actually you're wrong, we still use it there: getImagePageTitle(appLocalizations, _imageDataList[index].imageField) There would have been a warning if useless anyway.

@M123-dev @AshAman999 May you approve too? Not that I don't trust @omkarChend1kar, but for some reason I'm not able to merge even with one approval. Not enough seniority on this project maybe...

Yeah, Right missed it completely.

teolemon

The first option looks like a good improvement. Does it degrade gracefully if translated texts are longer ?
Were translations already handled, I don't see any additional translated strings.

monsieurtanuki · 2022-11-28T17:51:26Z

The first option looks like a good improvement. Does it degrade gracefully if translated texts are longer ?

You mean "edit before", right?
I'll check with lorem ipsum.

teolemon · 2022-11-28T17:55:35Z

Nothing special, 1 review needed to merge.

monsieurtanuki · 2022-11-28T18:00:33Z

Nothing special, 1 review needed to merge.

There seems to be different review colors, and the concept of "reviewer with write access":

monsieurtanuki · 2022-11-28T19:25:43Z

We cannot display more than 2 lines in the title, but the overflow can be handled "in fade mode":

Impacted files: * `new_crop_page.dart`: 2 lines for the title

monsieurtanuki · 2022-11-28T19:40:14Z

Thank you @omkarChend1kar @teolemon @AshAman999 for your help!

monsieurtanuki requested a review from a team as a code owner November 27, 2022 10:51

github-actions bot added 🖼️ Photos - Cropping 🥫 Product page labels Nov 27, 2022

github-actions bot assigned monsieurtanuki Nov 27, 2022

Merge branch 'develop' into feat/3332

59ae41b

Merge branch 'develop' into feat/3332

cb1f60b

monsieurtanuki mentioned this pull request Nov 28, 2022

One click addition of category-specific nutrients #3307

Open

4 tasks

omkarChend1kar approved these changes Nov 28, 2022

View reviewed changes

teolemon approved these changes Nov 28, 2022

View reviewed changes

monsieurtanuki added 3 commits November 28, 2022 20:26

feat: 3332 - 2 lines for the title

b328f29

Impacted files: * `new_crop_page.dart`: 2 lines for the title

Merge remote-tracking branch 'origin/feat/3332' into feat/3332

f430035

Merge branch 'develop' into feat/3332

1977488

monsieurtanuki merged commit 8831337 into openfoodfacts:develop Nov 28, 2022

monsieurtanuki mentioned this pull request Nov 28, 2022

Simplify adding photos by removing 2 of the 4 steps #3332

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: 3332 - crop page title now reflects image field #3366

feat: 3332 - crop page title now reflects image field #3366

monsieurtanuki commented Nov 27, 2022

monsieurtanuki commented Nov 27, 2022

codecov-commenter commented Nov 27, 2022 •

edited

Loading

monsieurtanuki commented Nov 27, 2022

omkarChend1kar commented Nov 28, 2022

AshAman999 commented Nov 28, 2022

omkarChend1kar Nov 28, 2022

monsieurtanuki commented Nov 28, 2022

omkarChend1kar commented Nov 28, 2022

teolemon left a comment •

edited

Loading

monsieurtanuki commented Nov 28, 2022

teolemon commented Nov 28, 2022

monsieurtanuki commented Nov 28, 2022

monsieurtanuki commented Nov 28, 2022

monsieurtanuki commented Nov 28, 2022

feat: 3332 - crop page title now reflects image field #3366

feat: 3332 - crop page title now reflects image field #3366

Conversation

monsieurtanuki commented Nov 27, 2022

What

Screenshot

Part of

monsieurtanuki commented Nov 27, 2022

codecov-commenter commented Nov 27, 2022 • edited Loading

Codecov Report

monsieurtanuki commented Nov 27, 2022

omkarChend1kar commented Nov 28, 2022

AshAman999 commented Nov 28, 2022

omkarChend1kar Nov 28, 2022

Choose a reason for hiding this comment

monsieurtanuki commented Nov 28, 2022

omkarChend1kar commented Nov 28, 2022

teolemon left a comment • edited Loading

Choose a reason for hiding this comment

monsieurtanuki commented Nov 28, 2022

teolemon commented Nov 28, 2022

monsieurtanuki commented Nov 28, 2022

monsieurtanuki commented Nov 28, 2022

monsieurtanuki commented Nov 28, 2022

codecov-commenter commented Nov 27, 2022 •

edited

Loading

teolemon left a comment •

edited

Loading