-
-
Notifications
You must be signed in to change notification settings - Fork 287
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: Add basic details page UI #1515
feat: Add basic details page UI #1515
Conversation
I've created the UI part and I have some doubts regarding the submission of data entered by the user. |
@bhattabhi013 this part should theoretically be not that hard because all querys to open food facts are routed to our other repository openfoodfacts-dart But actually @monsieurtanuki I'm unsure if the part is already done in off-dart, just did a quick check and we have But here:
I'm unsure if we add the |
@bhattabhi013 @M123-dev I think the calls you're referring to still need to be implemented, cf. openfoodfacts/openfoodfacts-dart#132 - assuming it's what we need here. |
We probably don't need the add_ prefix if we do things properly (we check for pre-existing data, and we offer the user the opportunity to correct or augment it) |
@bhattabhi013 Therefore you should use
|
Any updated @bhattabhi013 |
Hey @M123-dev,
|
Codecov Report
@@ Coverage Diff @@
## develop #1515 +/- ##
==========================================
- Coverage 8.86% 8.25% -0.62%
==========================================
Files 161 168 +7
Lines 6623 7333 +710
==========================================
+ Hits 587 605 +18
- Misses 6036 6728 +692
Continue to review full report at Codecov.
|
Hi everyone, Please let me know your thoughts on this and provide your input. |
@bhattabhi013 Could you add some margin left and right so that the fields do not look stuck to the edges of the screen ? |
Sure @teolemon, |
The carousel can stay edge to edge, since it's intended to be scrolled |
Hi @teolemon, |
LGTM visually 👍 |
@monsieurtanuki @M123-dev what about code-wise ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @bhattabhi013!
Many comments for you, lucky you :)
packages/smooth_app/lib/pages/product/add_basic_details_page.dart
Outdated
Show resolved
Hide resolved
packages/smooth_app/lib/pages/product/add_basic_details_page.dart
Outdated
Show resolved
Hide resolved
packages/smooth_app/lib/pages/product/add_basic_details_page.dart
Outdated
Show resolved
Hide resolved
if (widget.product.barcode != null) | ||
Padding( | ||
padding: EdgeInsets.symmetric(horizontal: size.width * 0.05), | ||
child: Column( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if adding a Column
inside a ListView
is a good idea. What about ListView
children?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used padding
widget to add some extra padding for the textfields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a Column
inside a ListView
adds complexity and possible side-effects. I strongly suggest that you use either a Column
or a ListView
, but not both.
], | ||
), | ||
), | ||
Row( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how you put the buttons cleanly at the bottom of the page. Here they are just after the rest, aren't they?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used Row to horizontally align the buttons and used MainAxisAlignment.spaceAround
value in mainAxisAlignment
for spacing purposes. Please suggest if you have something else in your mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern is about the vertical position of the Row. Let's keep it that way.
packages/smooth_app/lib/pages/product/add_basic_details_page.dart
Outdated
Show resolved
Hide resolved
packages/smooth_app/lib/pages/product/add_basic_details_page.dart
Outdated
Show resolved
Hide resolved
packages/smooth_app/lib/pages/product/add_basic_details_page.dart
Outdated
Show resolved
Hide resolved
packages/smooth_app/lib/pages/product/add_basic_details_page.dart
Outdated
Show resolved
Hide resolved
Indeed 😁. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @bhattabhi013!
Still not convinced by the ListView / Column combination. But that's good enough I guess.
if (widget.product.barcode != null) | ||
Padding( | ||
padding: EdgeInsets.symmetric(horizontal: size.width * 0.05), | ||
child: Column( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a Column
inside a ListView
adds complexity and possible side-effects. I strongly suggest that you use either a Column
or a ListView
, but not both.
], | ||
), | ||
), | ||
Row( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern is about the vertical position of the Row. Let's keep it that way.
We need to update the branch for it to be mergable |
Perfect, thanks @bhattabhi013 |
What
Created add basic details page UI.
Video
basic_Details_page.mp4
Fixes bug(s)
Part of