-
Notifications
You must be signed in to change notification settings - Fork 0
Feat/Consent form fridge leaderboard #473
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: main
Are you sure you want to change the base?
Conversation
JohanHjelsethStorstad
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.
Good work. I think this form can be added together with the rest of the 'general user settings'.
There is also a random projectNext file added.... maybe rebase if the file is large
| @@ -0,0 +1,21 @@ | |||
| 'use client' | |||
| import { updateUserAction } from '@/services/users/actions' | |||
| import Form from '@/app/_components/Form/Form' | |||
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.
@/components
| @@ -0,0 +1,21 @@ | |||
| 'use client' | |||
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 file should be uppercased and have he same name as the function it exports
| export default function RegisterKioleskapLeaderboard({ | ||
| userId, | ||
| kioleskapLead, | ||
| }: { |
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.
use PropTypes semantics
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.
I also think (if this is a separate form) that the form name should include the word consent
| memberships Membership[] | ||
| credentials Credentials? | ||
| feideAccount FeideAccount? | ||
| kioleskapLead Boolean @default(false) |
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.
I think this field name is non-descriptive. It should at least include the word consent. Also: avoid abbreviating the name.
| <h2>Generelle Instillinger</h2> | ||
| <Image width={300} image={profile.user.image} /> | ||
|
|
||
| <RegisterKioleskapLeaderboard userId={profile.user.id} kioleskapLead={profile.user.kioleskapLead} /> |
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.
Since this form anyways just calls the updateUserAction the form should probably be for all updateable user fields and not its own form. I.e. all fields updateable through updateUser should be in the same form
| @@ -1,3 +1,5 @@ | |||
| import RegisterKioleskapLeaderboard from './kioleskapLeaderboard' | |||
|
|
|||
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.
whitespace
being shown in the Kioleskap Leaderboard.
fix: #439