[WIP] allow to complete nutrition facts #36
Conversation
|
great 👌 I've pinged Raphael, Stéphane and Arnaud in the #hunger-games channel. Can you join it ? |
src/NutritionValues/index.js
Outdated
| const [productsBacklog, setProductsBacklog] = useState([]); | ||
|
|
||
| useEffect(() => { | ||
| if (nbOfPages && productsBacklog.length < 6) { |
There was a problem hiding this comment.
Be careful, you use Math.floor to set the nb of pages, so it can be 0. even if count > 0. But the problem is that 0 is false in JS, so you should check for !== undefined or use Math.ceil (and also be careful about the edge case of count === 0)
There was a problem hiding this comment.
Also an early return here can be good to directly see that if nbOfPages if undefined, nothing is executed
src/NutritionValues/index.js
Outdated
| const nbOfPages = useNumberOfPages(); | ||
| const [products, setProducts] = useGetProduct(nbOfPages); | ||
|
|
||
| return products && products.length > 0 ? ( |
There was a problem hiding this comment.
You should use early return to avoid to much indentation of your main block (that will probably later be more complex). Also you should maybe add a display if products.length can be 0.
I suggest something like that (with more {} depending on your taste)
if (! products) return <p>Loading</p>
if (products.length === 0) return <p>No more product</p>
return (
<>
...
</>
)
src/NutritionValues/index.js
Outdated
| lang: product.lang, | ||
| }; | ||
| }); | ||
| setProductsBacklog([...productsBacklog, ...products]); |
There was a problem hiding this comment.
I can suggest:
setProductsBacklog(
productsBacklog.concat(
products.map(({ code, lang, image_nutrition_url }) => ({
code,
imageUrl: image_nutrition_url,
lang,
})
))
)At least try to avoid using let when not required, two const with a name like productsFormatted will be simpler because you don't need to track when the variable was updated
|
Also when adding a new UI a quick screenshot in the PR description is useful to better understand the css code :) |
|
To avoid having to validate multiple times the deletion of a possible nutriment, I propose the folowing design. Here, I'm not sure of the value for energy, then I don't fill it When I click on validate, a recap appears and let me the possibility to confirm or to come back for modification. |
|
👍 I will just put some margin between buttons (or have skip in grey to distinct the limit and imply skip as a secondary action) Nit: Have consistent labels for |
|
For now, server receives only filled entries. For deleted one, should I associate an empty string to them ? @ArnaudBarre Thanks for the feedback, I will correct it |
src/NutritionValues/nutriments.js
Outdated
| export default { | ||
| Energy: 'nutriment_energy', | ||
| Fat: 'nutriment_fat', | ||
| 'Sat FAt': 'nutriment_saturated-fat', |
There was a problem hiding this comment.
| 'Sat FAt': 'nutriment_saturated-fat', | |
| 'Sat Fat': 'nutriment_saturated-fat', |
Not quite sure the capitalization was intentional
|
Can we merge this one and start testing it ? |
|
@teolemon Yes it's ready for test. Just for information, deletion does not work. I don't know how should I delete data. Should I send an empty string? I did'n found time to prefile with nutrient detection |
|
Yes, data deletion works with an empty string @alexfauquette |
|
@teolemon and @ArnaudBarre I think It can be merged like that (after reviewing last commit of course) I will be too busy this month to look at the "nutrient detection". We should probably add this as s distinct issue. |
this pull request is related to issue #17