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: #2396 - new widget SmoothProductCardTemplate #2593

Merged
merged 1 commit into from
Jul 14, 2022

Conversation

monsieurtanuki
Copy link
Contributor

New files:

  • smooth_product_card_template.dart: Empty template for a product card display.
  • smooth_product_image_container.dart: Container to display the main product image on a product card.

Impacted files:

  • smooth_product_card_found.dart: fix as backgroundColor was not taken into account anymore
  • smooth_product_image.dart: refactored with new class SmoothImageContainer

What

  • This is the first step of Safer and faster product list display #2396.
  • I cannot go further in Safer and faster product list display #2396 while this PR is not merged, and this PR is not very complicated, so a quick review would be appreciated ;)
  • As there are many files for this issue (20+), the idea is to split in small self-relevant chunks
  • Here in this PR, the main goal is to introduce a new widget: SmoothProductCardTemplate
  • This widget will be used each time we're loading a product from the database, in a product list display.
  • As with the issue we don't load all products first but load (and dismiss) them on demand, there are some cases when the scroll down will show products that are not loaded in memory yet.
  • The database load is always almost instant, but in the meanwhile we need to display something, in that case a SmoothProductCardTemplate.
  • That's a bit similar to youtube: they load a dozen of video previews, but when you scroll down they display empty templates while they're loading.

Screenshot

This is what SmoothProductCardTemplate looks like (as a test I display every other product with the template, but this is obviously not what will happen IRW):

dark light
Capture d’écran 2022-07-14 à 12 10 35 Capture d’écran 2022-07-14 à 12 11 23

Part of

New files:
* `smooth_product_card_template.dart`: Empty template for a product card display.
* `smooth_product_image_container.dart`: Container to display the main product image on a product card.

Impacted files:
* `smooth_product_card_found.dart`: fix as `backgroundColor` was not taken into account anymore
* `smooth_product_image.dart`: refactored with new class `SmoothImageContainer`
@monsieurtanuki monsieurtanuki requested a review from a team as a code owner July 14, 2022 10:16
Copy link
Member

@M123-dev M123-dev left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks @monsieurtanuki just a lot of magic numbers, I guess most of them are not introduced by you but taken from the original card, but they should still be a const or dynamic in other places.

Comment on lines +29 to +30
height: iconSize * .9,
width: 240 * iconSize / 130,
Copy link
Member

Choose a reason for hiding this comment

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

I guess you took these values from the original card. We should store them in a const

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough, but I don't know how to do that cleanly. You see, the 240x130 ratio comes from the svg files themselves.
The general idea is to have a template that looks similar to a real product card, not to match it in every detail, especially for a widget that is transient.

Comment on lines +23 to +24
width: screenSize.width * .4,
height: screenSize.width * .05,
Copy link
Member

Choose a reason for hiding this comment

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

Some magic numbers, what happenes when someone changes the font size

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't care about the font size, I just want 3 lines that fit in width * .2 as in the original card.

@monsieurtanuki monsieurtanuki merged commit 27681d1 into openfoodfacts:develop Jul 14, 2022
@monsieurtanuki
Copy link
Contributor Author

Thank you @M123-dev for your quick review!
I understand your concern about the template display and the magic numbers there; I replied to each comment.
Of course each time the original product card layout is getting modified we could modify that template too, but it does not happen too often, does it? And the template is really a transient display, an obvious "normally there should be a product there and it won't be long".
Of course if someone finds a solution to have a better (and systematic) match between the product card and the template, why not, but it's not a priority now.

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

Successfully merging this pull request may close these issues.

2 participants