-
Notifications
You must be signed in to change notification settings - Fork 285
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(auth): add create user modal to ui #4319
Conversation
disabled={!isDirty} | ||
// Only allow submission if the form is dirty for edits | ||
// When creating allow the user to click create without any changes as the form will be prefilled with valid values | ||
disabled={formMode === "edit" ? !isDirty : 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.
this was something i noticed, this !isDirty made it so a user had to make edits in the create flow - was originally added so that updates couldn't just be spam clicked
user { | ||
id | ||
} |
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.
@mikeldking i did not have luck with datasets in spreading fragments that fetched connections and getting the fragment to refetch so i did not add query type onto the return here and am instead using fetchKey - let me know if i'm missing someting but connections don't seem to be refetchable like that? https://relay.dev/docs/tutorial/mutations-updates/#adding-comments--mutations-on-connections - although users table doesn' thave the connection directive so maybe it would work here...
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.
Yeah I'm struggling with this as well. We can maybe use relay's optimistic updates instead. Then things will be better encapsulated
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.
yeah i'm not sure if optimistic updates work for connections, i might just try the connection directives - i had trouble last time but maybe i can get it to work here
@@ -67,6 +68,7 @@ export function UsersTable() { | |||
], | |||
data: tableData, | |||
getCoreRowModel: getCoreRowModel(), | |||
getSortedRowModel: getSortedRowModel(), |
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.
enables sorting to work, icons were showing up but wasn't sorting
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.
Left some comments but I think we can iterate on top of this!
import { UserRole } from "@phoenix/constants"; | ||
|
||
const UserRoles = Object.values(UserRole); |
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'd sorta prefer to make this use the API to get the roles. It might be some overhead right now but ultimately a tad more flexible and honest
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.
That works for me, i can make a follow up to do that
user { | ||
id | ||
} |
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.
Yeah I'm struggling with this as well. We can maybe use relay's optimistic updates instead. Then things will be better encapsulated
export enum UserRole { | ||
ADMIN = "ADMIN", | ||
MEMBER = "MEMBER", | ||
} |
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 always debate whether or not enums for this kinda thing is worth the effort. I know we do it a fair amount - non-blocking but when something can be expressed as string literal types very simply it begs the question a bit
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'll pull from the server - then we can get rid of this I think
resolve #4278
Screen.Recording.2024-08-21.at.4.42.03.PM.mov