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: implement nutrition data display (see #124) #136

Closed
wants to merge 1 commit into from
Closed

feat: implement nutrition data display (see #124) #136

wants to merge 1 commit into from

Conversation

muety
Copy link
Contributor

@muety muety commented Feb 25, 2024

Implement frontend-side part of #124. Tests are failing, but not sure how to go about them. Perhaps you could give me a quick intro on how testing is being done in this project?

Let me know what you think.

@@ -482,36 +484,48 @@ Additive? _convertAdditive(Enum$Additive e) {
Enum$Additive.LAXATIVE_IF_OVERUSED => Additive.laxativeIfOverused,
Enum$Additive.PHENYLALANINE => Additive.phenylalanine,
Enum$Additive.ALCOHOL => Additive.alcohol,
Enum$Additive.PRESSED_MEET => Additive.pressedMeat,
Enum$Additive.PRESSED_MEAT => Additive.pressedMeat,
Copy link
Contributor Author

@muety muety Feb 25, 2024

Choose a reason for hiding this comment

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

nutrition/logic seems to have diverged from main and didn't compile. Didn't want to rebase, though, because would probably be a mess when merging everything (backend and frontend) together, so I manually applied a couple of changes like this to get a working app to start with.

@uglrl
Copy link
Contributor

uglrl commented Feb 26, 2024

Could you maybe open a branch, so that we can run the tests here? I cannot run them on your fork. Then we can see why the tests are failing.

@muety
Copy link
Contributor Author

muety commented Feb 26, 2024

I don't think I have permission to create branches in this repo. But you can just clone my code and run the tests locally.

@worldofjoni
Copy link
Member

@muety Nice! I will take a closer look at that tomorow (after my exam 😉)

@worldofjoni worldofjoni deleted the branch kronos-et-al:nutrition/logic February 27, 2024 16:31
@worldofjoni
Copy link
Member

Ok, some organization points first:
Our branch structure may be a bit intransparent at the moment.
The initial idea was to have a monorepo but frontend and backend development separated on a frontend and backend branch, which then occasionally gets merged into main and updated from there. That worked ok for PSE, but I think now that leads more to inconsistencies that it provides structure.
Therefore there might not be the most recent front-end code on nutrution_data (a backend-based branch) than on frontend and main.
I am proposing to abolish this concept. Until then you might want to put your changes on top of the current frontend branch and then create a PR into that. But there you currently would not have the most up to date backend changes.

If you like, you can also add a section for environmental information.

Now to the content:
I think it would be nice to have the nutrition information arranged in a table like usual.
Furthermore, we could think about creating an extra section (at the end?) just for nutritional information. That, however, would only work for main dishes; sides we would then have to ignore or put their information in the accordion---or you have a different idea.
On the implementation side, your changes are hard to make out under the parts you picked from the frontend. From what I could see it looks mostly fine to me but @AlexKutschera should also take a look at that.

For the tests, maybe @AlexKutschera or @ElenaHaeussler can help you.

Recommended further proceeding

  • I will merge the backend changes into backend and then into main
  • you can move your changes (with hopefully some git magic) on top of the current frontend, or wait until the backend changes are merged and base your changes on top of that.

@muety
Copy link
Contributor Author

muety commented Feb 27, 2024

If you like, you can also add a section for environmental information.

Thought about that as well, but would probably prefer to do that in a separate pass.

I think it would be nice to have the nutrition information arranged in a table like usual.

Will do!

Furthermore, we could think about creating an extra section (at the end?) just for nutritional information.

I think from a UX perspective, the nutrition info goes quite well alongside additives and allergens, so would probably keep it inside the accordion. I didn't implement nutrition stats for sides, though. Will add that in soon.

[...] or wait until the backend changes are merged and base your changes on top of that.

Waiting for the backend will probably be less of a chaos, since I'll need the latest backend-side changes anyway, so will go for the second option. It's the #128, correct? Should be resolved soon-ish, I presume?

Looking forward to get your feedback on my implementation and how to proceed with the tests, @AlexKutschera, @ElenaHaeussler.

@ElenaHaeussler
Copy link
Contributor

ElenaHaeussler commented Feb 27, 2024

I will not have time to look at it before the end of next week. Maybe @AlexKutschera has time before then.

@worldofjoni
Copy link
Member

worldofjoni commented Feb 27, 2024

but would probably prefer to do that in a separate pass.

alright

from a UX perspective, the nutrition info goes quite well alongside additives and allergens

👍

It's the #128, correct? Should be resolved soon-ish, I presume?

yes. I hope so, but in the end, reviews take always longer than expected 😅

@worldofjoni
Copy link
Member

Sometimes it can also go fast: the new backend version is already merged to main and deployed

@muety
Copy link
Contributor Author

muety commented Feb 29, 2024

Awesome! I won't be available for about a week, though, so don't be surprised if I'll only continue working on this afterwards.

@muety
Copy link
Contributor Author

muety commented Mar 14, 2024

Just as a quick sign of life from my side: haven't forgot about this! Still keen to get this finished, it's just been a super busy week. Will do the remaining implementation somewhere in the next couple of days.

@AlexKutschera
Copy link
Collaborator

Hab deinen Branch mal auf https://github.com/kronos-et-al/MensaApp/tree/nutrition/app kopiert und aus der Liste eine Tabelle gemacht, passt für den Fall besser. Muss noch die Farben fixen, die tun aktuell noch nicht ganz was ich von ihnen will

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.

5 participants