-
Notifications
You must be signed in to change notification settings - Fork 909
refactor(site): improve minor queries and visuals on external auth #11066
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
Conversation
@kylecarbs @Emyrk just marking you to share a little bit of how we are organizing the queries and mutations |
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.
The table spacing is a lot better 👍
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.
Looks good!
Had one concern about the onSuccess
callback, but I'll leave it to you to decide if that's worth addressing now, or when we make the migration to RQ v5
queryKey: ["external-auth", providerId, "device", deviceCode], | ||
onSuccess: async () => { | ||
// Force a refresh of the Git auth status. | ||
await queryClient.invalidateQueries(["external-auth", providerId]); |
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 don't know what a good solution would be off the top of my head, but my main worry here is that onSuccess
has been deprecated for useQuery
in React Query v5, so I feel like we shouldn't become more dependent on that API, even though we can still technically use it in v4
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 is absolutely true! Let me remove it.
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.
After taking a look into this specific query, I think it is not trivial to remove it right now but we definitely can get back to this when migrating to v5.
PS: To really improve the design of this, I would need to refactor the settings layout a bit which is out of the scope for now.
Before:

Now:
