-
Notifications
You must be signed in to change notification settings - Fork 17
Add new bar menu #117
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
base: development
Are you sure you want to change the base?
Add new bar menu #117
Conversation
… redefining it manually
the implementation details were quite ugly, and if desired, a category called "Uncategorized" could just be created
the problem was the form state being in multiple places that could de-sync (I think) remedied this by hoisting all the state up to the `Product` and `NewProduct` respectively
|
I think this is ready for review now. Some implementation notes:
|
Sebbben
left a comment
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.
Overall looks really good.
There are two reoccuring problems in the api code i would like to be fixed.
Otherwise there are some small things, but all in all great work!
I somehow keep missing them
…er has interacted with it once
|
Just going to write down some improvements that could be implemented in the future (so I don't forget):
|
|
Thanks for the review! Going to request a review of the new changes :) |
Sebbben
left a comment
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.
Think this is mostly ready to merge. I would like to create a new issue to add optimistic updates to the UI, but i dont feel it's a priority in this PR unless you really feel like implementing it right away
…ating a new category
the mobile view might be getting somewhat crowded
pvk05
left a comment
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.
This pr looks very good. There are only a few things i want to comment on. Only admins and boardmembers should be able to edit the menu. Because of this, there needs to be a role check on both the page and the endpoints used for editing. The link for the editing page can be placed on the board page.
I dont think it is necessary to display the volunteer price, so that is fine. The MenuProducts table has outdated products anyway, so it is fine to drop.
The most important thing is the role access permissions. When that is implemented, i think it can be merged.
Overall, very good work
:) |
Closes #49
Very work in progress. Not ready for review or merging yet.TODO:
Current implementation:
Volunteer view (slightly outdated):

Customer view:
