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.

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

feat: Create a screen listing all attributes for a product #4714

Merged
merged 10 commits into from
Nov 4, 2023

Conversation

Sudhanva-Nadiger
Copy link
Contributor

@Sudhanva-Nadiger Sudhanva-Nadiger commented Oct 16, 2023

What

  • I have just added a simple button in edit product page just to test the new page. It will be removed and added in the paticular screen.
  • taking edit_product_page.dart as a base product_attribute_page.dart is designed as suggested by teolemon.
  • Added all the mentioned section field in the issue.

Screenshot

Part of

- resolves: openfoodfacts#4673
- I have just added a simple button in edit product page just to test the
new page. It will be removed and added in the paticular screen.
- taking `edit_product_page.dart` as a base `product_attribute_page.dart`
is designed as suggested by teolemon.
- Added all the mentioned section field in the issue.
Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

Hi @Sudhanva-Nadiger, and thank you for your PR!
I have many comments: please have a look at them. And fix the 2 lint check errors, too.
I'll have more comments later, but I need you to refactor the code first.

@monsieurtanuki
Copy link
Contributor

#4673 (comment)

@g123k Next time please don't involve me in an issue if you're already reviewing the PR yourself; I have already enough work to do.
@Sudhanva-Nadiger I switch to other tasks so feel free to ignore my comments.

- created seperte file for `svg_icon.dar`
- created newFile for `attribute_first_row_widget.dart`
- removed refreshIndicator
- useed stringbuffer for string concatenation
- fix: linting errors
@g123k
Copy link
Collaborator

g123k commented Oct 16, 2023

#4673 (comment)

@g123k Next time please don't involve me in an issue if you're already reviewing the PR yourself; I have already enough work to do. @Sudhanva-Nadiger I switch to other tasks so feel free to ignore my comments.

Oops, I'm really sorry, for me, my comments were complementary to yours.
@Sudhanva-Nadiger Please follow the comments from @monsieurtanuki. Once it's OK, it will give you my feedback about the UI.

@Sudhanva-Nadiger
Copy link
Contributor Author

Sudhanva-Nadiger commented Oct 16, 2023 via email

- to avoid recalculation on every build
@M123-dev
Copy link
Member

@monsieurtanuki what do you think code wise

Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

Hi @Sudhanva-Nadiger!
I'm sorry I couldn't follow your code changes. I'm more available now.
Could you please make sure that the tests are OK: https://github.com/openfoodfacts/smooth-app/actions/runs/6615121230/job/17966602417?pr=4714. That's the first step.

@codecov-commenter
Copy link

codecov-commenter commented Oct 31, 2023

Codecov Report

Merging #4714 (36fca5e) into develop (25068e5) will decrease coverage by 0.01%.
Report is 37 commits behind head on develop.
The diff coverage is 9.09%.

@@            Coverage Diff             @@
##           develop   #4714      +/-   ##
==========================================
- Coverage     9.90%   9.89%   -0.01%     
==========================================
  Files          310     312       +2     
  Lines        15808   15809       +1     
==========================================
- Hits          1566    1565       -1     
- Misses       14242   14244       +2     
Files Coverage Δ
...mooth_app/lib/pages/product/edit_product_page.dart 0.00% <ø> (-0.50%) ⬇️
...s/smooth_app/lib/generic_lib/widgets/svg_icon.dart 9.09% <9.09%> (ø)

... and 38 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

Hi @Sudhanva-Nadiger!

Thank you for your work!

Unfortunately you've put too much effort on something we actually don't need regarding UI. Please rollback

  • edit_product_page.dart
  • product_app_bar.dart
  • product_attributes_page.dart
    while keeping svg_icon.dart in a specific file as "not private anymore" (which makes sense).

Please create a new class StringPair with final String first and final String? second fields.
To be used instead of pasting 2 strings with a separator, as luckily we're coding in OOP and not in shell.

Please have a look at my comments for attribute_first_row_widget.dart.

Please create the file attribute_first_row_helper.dart with

  • abstract class AttributeFirstRowHelper in it, with method List<StringPair> getTerms();
  • class AttributeFirstRowSimpleHelper, that takes an AbstractSimpleInputPageHelper in the constructor
  • class AttributeFirstRowNutritionHelper, that takes the Product in the constructor
  • class AttributeFirstRowIngredientsHelper, that takes the Product in the constructor

With that you may say: "hey, we're not displaying anything!". That's correct. We'll deal with that once you're done with my suggestions.

Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

Thank you @Sudhanva-Nadiger, it's much better now!

My main new comment would be that in AttributeFirstRowHelper you should add something like that:

Future<void> onTap({
  required final BuildContext context,
  required final Product product,
})

In addition to that:

  • please remove product_app_bar.dart
  • please remove product_attributes_page.dart
  • please rollback edit_product_page.dart

This PR will focus on building the tools (basically AttributeFirstRowHelper and AttributeFirstRowWidget), without display.
If you don't mind once this PR is approved and merged we'll focus on where and when we should display the data.

@Sudhanva-Nadiger
Copy link
Contributor Author

Ok cool I will do that 👍🏻!

Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

@Sudhanva-Nadiger We're getting closer!
Please have a look at my comments, and we'll probably be good for an approved PR ;)

@Sudhanva-Nadiger
Copy link
Contributor Author

@monsieurtanuki done !

Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

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

Thank you @Sudhanva-Nadiger, that's very good now!

@monsieurtanuki
Copy link
Contributor

@g123k Please consider approving this excellent PR, as the merge button is blocked by your long-time-ago change request.

@monsieurtanuki monsieurtanuki dismissed g123k’s stale review November 4, 2023 14:19

Just trying, no offense.

@monsieurtanuki monsieurtanuki merged commit 21469af into openfoodfacts:develop Nov 4, 2023
7 checks passed
@monsieurtanuki
Copy link
Contributor

@g123k I didn't know I had the power to dismiss change requests. Given the context it looked like an appropriate action.
No hard feelings ;)

@g123k
Copy link
Collaborator

g123k commented Nov 4, 2023

@g123k I didn't know I had the power to dismiss change requests. Given the context it looked like an appropriate action. No hard feelings ;)

No problem!

@Sudhanva-Nadiger
Copy link
Contributor Author

I havent added the Expand word to the translation ! there is one more pr continuation of this pr i am working on it ! i will add that in the next pr !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants