Skip to content

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

Merged
merged 3 commits into from
Dec 6, 2023

Conversation

BrunoQuaresma
Copy link
Collaborator

  • Minor visual improvements
  • Minor query refactoring

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:
Screenshot 2023-12-06 at 13 39 57

Now:
Screenshot 2023-12-06 at 13 40 05

@BrunoQuaresma BrunoQuaresma changed the title Bq/refactor oauth settings visual refactor(site): minor query and visual improvements on external auth Dec 6, 2023
@BrunoQuaresma
Copy link
Collaborator Author

@kylecarbs @Emyrk just marking you to share a little bit of how we are organizing the queries and mutations

@BrunoQuaresma BrunoQuaresma changed the title refactor(site): minor query and visual improvements on external auth refactor(site): improve minor queries and visuals on external auth Dec 6, 2023
Copy link
Member

@Emyrk Emyrk left a 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 👍

Copy link
Member

@Parkreiner Parkreiner left a 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]);
Copy link
Member

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

https://tkdodo.eu/blog/breaking-react-querys-api-on-purpose

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.

@BrunoQuaresma BrunoQuaresma merged commit 667ee41 into main Dec 6, 2023
@BrunoQuaresma BrunoQuaresma deleted the bq/refactor-oauth-settings-visual branch December 6, 2023 19:17
@github-actions github-actions bot locked and limited conversation to collaborators Dec 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants