feat: Nutrition Game #70

[Shmigolk]

I added new page nutrition and made simple UI. I need your recommendation about UI, logic, etc.

[VaiTon]

I would add some basic infos about the product that is being edited, like name, brand, size, just to make sure that the info actually makes sense

[nobeeakon]

nice work, just added some comments, please don't hesitate to ask if you have any question

[nobeeakon]

these ids could be improved, what about something like 'dropdown-unit-label' ?... better to use something as specific as we can or we could end up having multiple similar ids

[nobeeakon]

think it would be nice to somewhere something that gives you an idea of what these values (10, 21, 22) actually mean. What about using an object outside of this component, something like const weightUnits = {g:10, mg:21, kg:22} (please note that I don't really understand what those values refer to and where do they come from). It is better to have some auto documentation rather than having some unkown values

[nobeeakon]

is this being used somewhere?

[nobeeakon]

is this package used?

[nobeeakon]

why is it useful to have this package? I see that it is kind of used in pages/nutrition/index.jsx but seems to be importing a non used flexbox (what would be the advantage of using that flexbox compared to css defined flexbox?)

[nobeeakon]

is sx={{}} needed?

[Shmigolk]

I removed checkbox and leaved delete icon. Still working on it

[alexfauquette]

Sounds already quite solid and well structured. I did not had time to have a look to the management of the table itself

[alexfauquette]

maybe set the index to 0, or keep the previous products with setPoducts(prevProducts => [...prevProducts, ...data.products])

[alexfauquette]

You are trying to keep up to date 2 different arrays which is error pron. As soon as you forget an edge case, the two arrays will not be sync and it's hard to debug

I propose to keep the setNutriments that could be renamed addNutrimentToTheTable
Replace the setAdditionalNutriments by missingNutriments which would be computed as follow:

const missingNutriments = offNutriments.filter(nutrimentKey => 
	data.every(element) => element.off_nutriment_id !== nutrimentKey) 
)

with offNutriments the list of all the nutrients available on openfoodfact

This would replace options and setAdditionalNutriments

[alexfauquette]

I think you can start by removing the @material-ui/system, react-virtualized, and clsx libraries.

And focus on the management of table rows

[alexfauquette]

Suggested change

import { flexbox } from "@material-ui/system";

imported but not used. Normally when you do yarn start in the console, you should see some warnings about lines where you import or define variables you do not use. You should remove them to get a cleaner code.

---

Here is what I get when I run yarn start from your branch

[alexfauquette]

duplicated

Suggested change

	{
	off_nutriment_id: "energy_kcal",
	label: "Shugar",
	value: "",
	unit: "null",
	quantification: "<",
	robotoffPrediction: null
	},

[alexfauquette]

Maybe rename it defaultNutrimentTableData Because it's the default values that will be edited by the user

[alexfauquette]

You need a way to say "there is no unit specified for this nutriment". It can be either null or an empty string, but not the string "null"

Suggested change

	unit: "null",
	unit: "",

Suggested change

	unit: "null",
	unit: null,

[alexfauquette]

It works, but writing offService.getNutritionToFillUrl() as an advantage. When reading the line of code we know this data is fetched from openfoodfact server. Otherwise it could be fetched from robotoff, or an other file

Suggested change

const { getNutritionToFillUrl } = offService

[alexfauquette]

Suggested change

	const productListUrl = getNutritionToFillUrl({page})
	const productListUrl = offService.getNutritionToFillUrl({page})

[alexfauquette]

I assume it's a mistake. The </Box> is planned to be before the coma But that's not important.

@Shmigolk I think you get confused by the MUI demo, because it's cells only contain strings/numbers, so they use

<TableCell>{row.thValueToDisplay}</TabCell>

In our case, we want to render inputs which is a bit more complex.

I propose you the following strategy:
create a component NutritionTableRow as follow:

const NutritionTableRow = ({ nutrimentData, updateValue, updateUnit, updateQuantificator }) => {
	const onNutrimentValueChange = (event) => {updateValue(event.target.value)}
	const onUnitChange = () => {...}
	const onQuantificatorChange = () => {...}
	
	const {label, value, quantification, unit} = nutrimentData
	
	return <TableRow>
		<TableCell>
			<TextField select value={quantification} onChange={onQuantificatorChange}>
				<MenuItem value="=">=</MenuItem>
				<MenuItem value=">">></MenuItem>
				<MenuItem value="<"><</MenuItem>
			</TextField>
		</TableCell>
		<TableCell>
			<TextField label={label} value={value} onChange={onNutrimentValueChange} />
		</TableCell>
		//  Same for the unit
	<TableRow>
}

Then you will be able to simplify your table content as follow:

	nutriments.map(nutriment => <NutritionTableRow nutrimentData={nutriment}  updateValue={updateUnit(nutriment.off_nutriment_id)}/>)

The trickiest part being updateValue which is a function returning a function.

const updateValue = (nutrimentId) => (newValue) => {
	// We get the index of the nutriment we want to update
	const indexToModify = nutriments.findIndex(nutriment => nutriment.off_nutriment_id === nutrimentId)
	
	// early return if not element correspond to this id
	if(indexToModify < 0){ return }
	
	// We update the value to the nutriment
	const newNutriment = {...nutriments[indexToModify], value: newValue }
	setNutriments([...nutriments.slice(0, indexToModify), newNutriment, ...nutriments.slice(indexToModify+1)])
}

[alexfauquette]

There is a shortcut for that by using TextField. It's

<TextField select value={...} onChange={...} label="unit">
	<MenuItem value={null}></MenuItem>
	<MenuItem value="g">g</MenuItem>
	<MenuItem value="mg">mg</MenuItem>
	...
</TextField>

[alexfauquette]

Normally with those props it should be enough. The page does not need to know what you're doing. it just care about having the correct data thanks to setNutriments

Suggested change

	export default function NutritionTable({nutriments, setNutriments, additionalNutriments, deleteItem, setAdditionalNutriments, onchangeHandler}) {
	export default function NutritionTable({nutriments, setNutriments }) {

[teolemon]

https://static.openfoodfacts.org/data/taxonomies/nutrients.json
https://world.openfoodfacts.org/cgi/nutrients.pl
https://world-fr.openfoodfacts.org/cgi/nutrients.pl

[alexfauquette]

Sounds like the previous review I did has not been saved.

[alexfauquette]

Here we discussed that you can use newValue to avoid some edge cases

Suggested change

	onChange={event => {
	onChange={(event, newValue) => {

[alexfauquette]

Suggested change

	<Box display={"flex"}
	<Box display="flex"

Some points which will be useful for tech interview about React (JSX syntax to be precise):

• By default props are provided as follow propName={propValue}
• You do not need the {} for sting. FOr example propName={"some text"} is same as propName="some text"
• A prop alone is equivalent to true. For example <Component isOpen /> is the same as <Component isOpen={true} />.

[nobeeakon]

Sounds this PR is increasing its size, I know it has been a lot of time invested on this, but I wonder if we could split what has been done here in smaller PRs with a limited and concrete objective each?, so that those changes can be reviewed and merged in a quicker way. I think #71 can be a good starting point. So, basically some of the things you already have in this PR @Shmigolk , but trying to tackle them in an step by step approach as this PR already has a lot of comments and is covering a number of things and features so more space for comments and becomes more time consuming to review. What do you guys think @Shmigolk , @alexfauquette ?