Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: 4674 - added the "other photos" section #4866

Merged
merged 5 commits into from
Dec 8, 2023
Merged

feat: 4674 - added the "other photos" section #4866

merged 5 commits into from
Dec 8, 2023

Conversation

monsieurtanuki
Copy link
Contributor

What

  • Added the "other photos" section to the product gallery page.
  • Those "other photos" are to be always network-downloaded at page init, until feat: simplified access to raw product images openfoodfacts-dart#839 is merged - "other photos" will then be included in the product saved locally in database.
  • Clicking on an "other photo" thumbnail opens a full screen page with that photo and that's it for the moment.

Screenshot

"other photos" added (top) "other photos" added (bottom) other photo page
Screenshot_1701351383 Screenshot_1701351407 Screenshot_1701351435

Part of

Files

New files:

  • product_image_gallery_other_view.dart: Display of the other pictures of a product.
  • product_image_other_page.dart: Full page display of a raw product image.

Impacted file:

  • product_image_gallery_view.dart: added the "other photos" section; fixed a "refresh not working" bug

@monsieurtanuki
feat: 4674 - added the "other photos" section
b9152bc 
New files:
* `product_image_gallery_other_view.dart`: Display of the other pictures of a product.
* `product_image_other_page.dart`: Full page display of a raw product image.

Impacted file:
* `product_image_gallery_view.dart`: added the "other photos" section; fixed a "refresh not working" bug
@monsieurtanuki monsieurtanuki requested a review from a team as a code owner November 30, 2023 14:04
@teolemon
Copy link
Member

teolemon commented Dec 2, 2023

  • Could you collapse at mid height of the second row of photos with some more button ?
  • This is still a "grand public" screen, so let's adopt something lighter than the future "Contribution" tab

@codecov-commenter
Copy link

codecov-commenter commented Dec 2, 2023 •
edited
Loading

Codecov Report

Attention: 81 lines in your changes are missing coverage. Please review.

Comparison is base (b91a784) 9.77% compared to head (545d4b5) 9.72%.

Files Patch % Lines
.../pages/image/product_image_gallery_other_view.dart 0.00% 41 Missing ⚠️
.../lib/pages/product/product_image_gallery_view.dart 0.00% 28 Missing ⚠️
..._app/lib/pages/image/product_image_other_page.dart 0.00% 12 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           develop   #4866      +/-   ##
==========================================
- Coverage     9.77%   9.72%   -0.05%     
==========================================
  Files          313     315       +2     
  Lines        15976   16044      +68     
==========================================
  Hits          1561    1561              
- Misses       14415   14483      +68

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

@monsieurtanuki
Copy link
Contributor Author

  • Could you collapse at mid height of the second row of photos with some more button ?
  • This is still a "grand public" screen, so let's adopt something lighter than the future "Contribution" tab

Generally speaking, would displaying the main 4 pictures, then a "more photos" button that downloads then displays the "other" pics do the trick?

@teolemon
Copy link
Member

teolemon commented Dec 2, 2023

Yes, at this point 👍

@monsieurtanuki
Copy link
Contributor Author

@teolemon Coded and pushed: the first time the user goes to that page we display a button instead of the whole list of photos.
Screenshot_1701534205

@teolemon
Copy link
Member

teolemon commented Dec 3, 2023

There could be a confusion with a photo button taking button. Let's rephrase the button to "View all existing photos for this product"

@monsieurtanuki
feat: 4674 - different label for button
5495fc1 
Impacted files:
* `app_en.arb`: add new label for "click for other photo" button
* `app_fr.arb`: add new label for "click for other photo" button
* `product_image_gallery_view.dart`: used new label for "click for other photo" button
@monsieurtanuki
Copy link
Contributor Author

There could be a confusion with a photo button taking button. Let's rephrase the button to "View all existing photos for this product"

@teolemon Coded and pushed:
Screenshot_1701625852

@g123k
Copy link
Collaborator

g123k commented Dec 4, 2023

I'm not a big fan of this button.
I would keep the label from your initial PR "Other photo" and just add a button "Download/fetch photos".

We need to keep a hierarchy and not mix different things.
Also, shadows for the FAB and this button VS nothing for the picture doesn't give the click that actually the photos are clickable.

@monsieurtanuki
Copy link
Contributor Author

I'm not a big fan of this button.
I would keep the label from your initial PR "Other photo" and just add a button "Download/fetch photos".

@g123k In those cases, please approve the PR first and then I'll do the minor changes. With just a couple of reviewers at the moment, we need to remove frictions in the PR approval process in order to have smoother code integration. Assuming that smoother code integration is a good idea.

g123k
g123k reviewed Dec 8, 2023
View reviewed changes
packages/smooth_app/lib/pages/image/product_image_gallery_other_view.dart
final AppLocalizations appLocalizations = AppLocalizations.of(context);
final double screenWidth = MediaQuery.of(context).size.width;
final double squareSize = screenWidth / _columns;
return FutureBuilder<List<int>>(
Copy link
Collaborator

@g123k g123k Dec 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh no, please don't use a FutureBuilder, that's the shittiest Widget ever.
A separated class with a ValueNotifier-like thing or I don't know which state management solution, but never mix UI with how the data is fetched.

I know the app uses this pattern everywhere, but if I can stop the spreading of this 🦠, I prefer.

Edit: If you don't see what I mean, I can eventually edit your PR

@monsieurtanuki monsieurtanuki mentioned this pull request Dec 8, 2023
Open
@teolemon
Copy link
Member

teolemon commented Dec 8, 2023

  • @monsieurtanuki do you want to handle the FeatureBuilder now or should I add it to the Backlog ?
  • I'm going to open an issue to get closer to the clean design of the PoC, and add image management feature (like reporting bad images, timestamps, uploaders and more…)

@monsieurtanuki
Copy link
Contributor Author

  • @monsieurtanuki do you want to handle the FeatureBuilder now or should I add it to the Backlog ?
  • I'm going to open an issue to get closer to the clean design of the PoC, and add image management feature (like reporting bad images, timestamps, uploaders and more…)

@teolemon The FutureBuilder replacement is already tracked in #4882 so you don't have to add it to the backlog - and I feel free from coding a replacement in the current PR.
Please write precisely what is missing functionally in a separate issue if needed.

@monsieurtanuki monsieurtanuki merged commit f0c26da into openfoodfacts:develop Dec 8, 2023
6 checks passed
@monsieurtanuki
Copy link
Contributor Author

Thank you guys for your comments and reviews!

@github-actions github-actions bot mentioned this pull request Dec 8, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@g123k g123k g123k left review comments

@teolemon teolemon teolemon approved these changes

Assignees

@monsieurtanuki monsieurtanuki

Labels
🌐 l10n 🥫 Product page
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@monsieurtanuki @teolemon @codecov-commenter @g123k