feat: Ingredients Page #707
Conversation
I think you just update this file with a new string.
See example: #647
Working with PM/UX to get this straightened out by this week.
Oh I'm not sure, I think it might actually be a bug, I tried it using the smoothie app on my phone and the behavior was as you described. I created #709
Stephane's response |
teolemon
added
the
Product addition
github-actions
bot
removed
the
Product addition
justinmc
force-pushed
the
edit-ingredients
branch
from
ce3ab02 to
65b8fad
Compare
|
@M123-dev I think this is ready to be reviewed and get merged. I put everything behind a dev mode flag so it shouldn't break anything. It just hides a button in the ingredient knowledge panel that looks like this:
I may be missing some coding style things, or using the appropriate themes or constants in places, let me know what I can clean up. |
There was a problem hiding this comment.
Hi @justinmc!
Many things here and there that could be refactored.
The most worrying thing is that I have the impression that the final button ("confirm") does nothing.
Please have a look at my comments.
| @@ -336,6 +336,10 @@ | |||
| }, | |||
| "ingredients": "Ingredients", | |||
| "@ingredients": {}, | |||
| "ingredients_editing_instructions": "Conserver l'ordre, indiquer le % lorsqu'il est précisé, séparer par une virgule ou -, utiliser les () pour les ingrédients d’un ingrédient, indiquer les allergènes entre _.", | |||
There was a problem hiding this comment.
Should be in English instead of French.
There was a problem hiding this comment.
I got this French text from a mock. I'll give it my own best English translation.
| //import 'package:openfoodfacts/model/Product.dart'; | ||
| //import 'package:smooth_app/data_models/product_preferences.dart'; |
| final bool isUploaded = await uploadCapturedPicture( | ||
| context, | ||
| barcode: widget.product | ||
| .barcode!, //Probably throws an error, but this is not a big problem when we got a product without a barcode |
There was a problem hiding this comment.
Please remove that comment: it's widely assumed that we don't care about barcode-less products.
There was a problem hiding this comment.
Sounds good, I think I got this code originally from image_upload_card.dart but it's since changed.
| final OpenFoodFactsLanguage? language = ProductQuery.getLanguage(); | ||
| if (language == null) { | ||
| throw Exception("Couldn't find language."); | ||
| } |
There was a problem hiding this comment.
Just assume it is not null and put a !.
| }); | ||
| } | ||
|
|
||
| Future<void> _getImage() async { |
There was a problem hiding this comment.
You should comment and make clear that this method throws exceptions from which an error message will be displayed.
As those messages will be displayed, they should be localized.
There was a problem hiding this comment.
Only a generic error message is shown to the user, and that one is already localized (ingredients_editing_image_error).
| return Row( | ||
| mainAxisAlignment: MainAxisAlignment.end, | ||
| children: <Widget>[ | ||
| if (!hasImage) |
There was a problem hiding this comment.
Again, it would be easier to read with something like return hasImage ? Row(...) : Row(...).
There was a problem hiding this comment.
Done, and pulled out a variable for the Stack children.
| onPressed: getImage, | ||
| child: const Icon(Icons.refresh), | ||
| ), | ||
| if (hasImage) const SizedBox(width: 12.0), |
There was a problem hiding this comment.
Use MEDIUM_SPACE from design_constants.dart instead.
There was a problem hiding this comment.
Thanks, I figured something like that file existed but never noticed it.
| backgroundColor: Colors.blue, | ||
| foregroundColor: Colors.white, |
There was a problem hiding this comment.
The colors should match the user theme.
| tooltip: 'Confirm', | ||
| backgroundColor: Colors.blue, | ||
| foregroundColor: Colors.white, | ||
| onPressed: () {}, |
There was a problem hiding this comment.
Nothing happens here?
There was a problem hiding this comment.
Whoops, should return to the previous page.
| Flexible( | ||
| flex: 1, | ||
| child: Container( | ||
| height: 400.0, |
There was a problem hiding this comment.
Where does 400 come from? Please add a TODO if it's a temporary guess or use a constant from design_constants.dart.
There was a problem hiding this comment.
Whoops, it's irrelevant now, it's sized by the Flexible above. Will remove it.
| if (ingredients == null) { | ||
| return ''; | ||
| } | ||
| String string = ''; |
There was a problem hiding this comment.
Also a StringBuffer would be much better here, since you concatenate multiple strings
| for (int i = 0; i < ingredients.length; i += i) { | ||
| final Ingredient ingredient = ingredients[i]; | ||
| if (i == ingredients.length - 1) { | ||
| string += ingredient.toString(); | ||
| } else { | ||
| string += ', $ingredient'; | ||
| } | ||
| } |
There was a problem hiding this comment.
Mm or maybe directly, the join(', ') method?
|
@g123k @monsieurtanuki Thanks for the review! I think I've responded to everything. |
There was a problem hiding this comment.
Hi @justinmc, it looks better that way!
Still I have micro-comments that you can read.
Regarding the use of LoadingDialog I cannot really say what is expected precisely for this app UX-wise, I can only say the way I expect an app to behave (I'm old-fashioned). Besides, as there are recent open issues regarding offline product edit I guess whatever you could code here will soon be affected, so we should not put too much attention on details.
| ? <Widget>[ | ||
| FloatingActionButton.small( | ||
| tooltip: 'Retake photo', | ||
| backgroundColor: Theme.of(context).buttonTheme.colorScheme!.background, |
There was a problem hiding this comment.
I guess you could create and reuse a variable with Theme.of(context).buttonTheme.colorScheme!.
| FloatingActionButton.small( | ||
| tooltip: 'Confirm', | ||
| backgroundColor: Theme.of(context).buttonTheme.colorScheme!.primary, | ||
| foregroundColor: Theme.of(context).buttonTheme.colorScheme!.onBackground, |
There was a problem hiding this comment.
Shouldn't it rather be onPrimary?
| return Align( | ||
| alignment: Alignment.bottomLeft, | ||
| child: Padding( | ||
| padding: const EdgeInsets.symmetric(horizontal: 16.0), |
There was a problem hiding this comment.
Please remove all you "personal" paddings (like 16.0) and use constants from design_constants.dart instead.
| } | ||
| } | ||
|
|
||
| // The actions for the page in a row of FloatingActionButtons. |
There was a problem hiding this comment.
Here a /// comment makes more sense than a mere //.
| @@ -336,6 +336,10 @@ | |||
| }, | |||
| "ingredients": "Ingredients", | |||
| "@ingredients": {}, | |||
| "ingredients_editing_instructions": "Keep the original order. Indicate the percentage when specified. Separate with a comma or hyphen, use parentheses for ingredients of an ingredient, and indicate allergens between underscores.", | |||
There was a problem hiding this comment.
I guess you could provide values for app_fr.arb too.
| this.barcode, | ||
| }) : super(key: key); | ||
|
|
||
| final String? barcode; |
There was a problem hiding this comment.
What's the point of this barcode? If there's a clear use-case where it should not be product.barcode, please add a comment; if not, please remove this field.
There was a problem hiding this comment.
Good catch, I think I had forgotten to remove this when I added product to the parameters.
| }); | ||
| } | ||
|
|
||
| Future<void> _onTapGetImage() async { |
There was a problem hiding this comment.
What about LoadingDialog here?
There was a problem hiding this comment.
It's being used when I call out to picture_capture_helper and product_refresher here (see this comment and the video there).
| barcode: widget.product | ||
| .barcode!, |
There was a problem hiding this comment.
Not sure why I did that 😁
|
Thanks for all the help on this PR! 🎉 |
|
Thanks @justinmc I've just triggered the playstore alpha build 🎉 |
|
Thanks @justinmc, looking foreward to testing it out |
Related issues
My attempt at the ingredients input page. I'm struggling to put together a full PR, so I'll plan to ask a bunch of questions about the mocks, process, etc. and point people to this PR.
How it looks currently
Mocks I've been using
From Figma: https://www.figma.com/file/WXBmNSPweKqXpoSNRSQWDc/Product-Addition---Nutrition?node-id=0%3A1
And from the issues:
Changes from this PR
That's it! Is there anywhere else that's supposed to link to the EditIngredientsPage or anything else I'm missing in this flow? Below I have many other specific questions for me to move forward.
Questions
Process
Design/product
Tapping the ingredients image at the top of the product page goes to a photo view of the ingredients. Should that remain, or somehow be combined with this "edit ingredients" flow? Am I missing any part of the flow/navigation for editing ingredients?
What is the light grey text that is seen below the ingredients TextField in the mock?
On my EditIngredientsPage pictured farther above, I used BoxFit.cover for the image fit, which results in wide images like the one pictured being cut off. Is that ok, or should I use BoxFit.contain or some other layout?
Engineering