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: 4628 - refactoring as preparatory work #4641

Merged
merged 7 commits into from
Oct 28, 2023

Conversation

monsieurtanuki
Copy link
Contributor

What

  • The issue demands some understanding of the knowledge panels, and for that I had to delve into the code.
  • En passant I refactored a bit the code.
  • Now we have a clearer code and I have clearer thoughts about knowledge panels!
  • The next step is to actually reorder the knowledge panels according to the user choice (with a reorderable list view). Not really a technical problem, but that's not part of the current PR.

Part of

Files

Deleted files:

  • knowledge_panel_element_card.dart
  • knowledge_panel_summary_card.dart

New files:

  • knowledge_panel_image_card.dart: Card that displays a Knowledge Panel Image element.
  • knowledge_panel_text_card.dart: Card that displays a Knowledge Panel Text element.
  • website_card.dart: Card that displays a website link. Code used to be inside new_product_page.dart

Impacted files:

  • knowledge_panel_card.dart: minor refactoring
  • knowledge_panel_expanded_card.dart: minor refactoring
  • knowledge_panel_page.dart: minor refactoring
  • knowledge_panel_page_template.dart: minor refactoring
  • knowledge_panels_builder.dart: moved code here from knowledge_panel_element_card.dart and knowledge_panel_summary_card.dart; minor refactoring
  • new_product_page.dart: moved the WebsiteCard code to a dedicated file; minor refactoring
  • product_cards_helper.dart: minor refactoring
  • summary_card.dart: minor refactoring

Deleted files:
* `knowledge_panel_element_card.dart`
* `knowledge_panel_summary_card.dart`

New files:
* `knowledge_panel_image_card.dart`: Card that displays a Knowledge Panel _Image_ element.
* `knowledge_panel_text_card.dart`: Card that displays a Knowledge Panel _Text_ element.
* `website_card.dart`: Card that displays a website link. Code used to be inside `new_product_page.dart`

Impacted files:
* `knowledge_panel_card.dart`: minor refactoring
* `knowledge_panel_expanded_card.dart`: minor refactoring
* `knowledge_panel_page.dart`: minor refactoring
* `knowledge_panel_page_template.dart`: minor refactoring
* `knowledge_panels_builder.dart`: moved code here from `knowledge_panel_element_card.dart` and `knowledge_panel_summary_card.dart`; minor refactoring
* `new_product_page.dart`: moved the `WebsiteCard` code to a dedicated file; minor refactoring
* `product_cards_helper.dart`: minor refactoring
* `summary_card.dart`: minor refactoring
@monsieurtanuki monsieurtanuki requested a review from a team as a code owner September 17, 2023 16:38
@github-actions github-actions bot added 🥫 Product page Product addition The easier it is to add a product and get Nutri-Score, Eco-Score, the happier the users. 🤗 Onboarding We need to onboard users on how the app works, but also on content like Nutri-Score, Eco-Score… summary card labels Sep 17, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 17, 2023

Codecov Report

Merging #4641 (4d2d9bd) into develop (2d90250) will increase coverage by 0.00%.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##           develop   #4641   +/-   ##
=======================================
  Coverage     9.90%   9.90%           
=======================================
  Files          310     311    +1     
  Lines        15807   15796   -11     
=======================================
- Hits          1566    1565    -1     
+ Misses       14241   14231   -10     
Files Coverage Δ
...e_panel/knowledge_panels/knowledge_panel_page.dart 0.00% <0.00%> (ø)
...ages/onboarding/knowledge_panel_page_template.dart 0.00% <0.00%> (ø)
...ges/smooth_app/lib/pages/product/summary_card.dart 0.00% <0.00%> (ø)
...smooth_app/lib/pages/product/new_product_page.dart 0.00% <0.00%> (-0.47%) ⬇️
...s/smooth_app/lib/helpers/product_cards_helper.dart 0.00% <0.00%> (ø)
...l/knowledge_panels/knowledge_panel_image_card.dart 0.00% <0.00%> (ø)
...nowledge_panels/knowledge_panel_expanded_card.dart 0.00% <0.00%> (ø)
...e_panel/knowledge_panels/knowledge_panel_card.dart 0.00% <0.00%> (ø)
...el/knowledge_panels/knowledge_panel_text_card.dart 0.00% <0.00%> (ø)
...ges/smooth_app/lib/pages/product/website_card.dart 0.00% <0.00%> (ø)
... and 1 more

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

Copy link
Collaborator

@g123k g123k left a comment

Choose a reason for hiding this comment

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

Just some minor comments

}

String _getWebsite() =>
!website.startsWith('http') ? 'http://$website' : website;
Copy link
Collaborator

Choose a reason for hiding this comment

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

That http is bothering me, what are your thoughts on switching to https?

@monsieurtanuki
Copy link
Contributor Author

Just some minor comments

Hi @g123k!
Your minor comments are on parts of the code I did not write, but only moved during this refactoring.
I cannot go back to details of old code for minor comments, based on "wouldn't it be slightly better if" assumptions.
Especially with my limited time / access to a computer.
The best I can do is add TODOs.

@monsieurtanuki
Copy link
Contributor Author

@g123k I've just added TODOs for the minor remarks you wrote during your first review.

@monsieurtanuki monsieurtanuki merged commit e463ca3 into openfoodfacts:develop Oct 28, 2023
6 checks passed
@monsieurtanuki
Copy link
Contributor Author

Thank you @g123k for your review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤗 Onboarding We need to onboard users on how the app works, but also on content like Nutri-Score, Eco-Score… Product addition The easier it is to add a product and get Nutri-Score, Eco-Score, the happier the users. 🥫 Product page summary card
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants