-
Notifications
You must be signed in to change notification settings - Fork 886
chore(site): refactor groups to use react-query #9701
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
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 update makes sense to me. The big question I had was about having multiple children share a common mutation source in GroupPage.tsx
, though. I don't know if that has a risk of causing bugs
@@ -0,0 +1,101 @@ | |||
import { QueryClient } from "@tanstack/react-query"; | |||
import * as API from "api/api"; |
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.
Following the conversation we had earlier, would this need to be updated to use named imports, or do we just generally do the wildcard import for the API?
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 is a good question, I like this way because it avoids conflicts but I don't think we set a preference. What do you think @aslilac ?
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, this is what I've been doing to. it's fine. 🤷♀️
}; | ||
}; | ||
|
||
export const group = (groupId: string) => { |
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 can't remember if anyone discussed this, but is there a naming convention that we're going to follow for these React Query utility functions?
I guess my concern here is that group
can be a verb or a noun, so it isn't immediately clear whether it does grouping, or if it creates a config object relevant to groups
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.
Hm...... this is a good one, I don't have an answer for that. What do you think @Parkreiner @aslilac ?
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.
Maybe something like configureGroupQuery
and configureGroupMutation
? That way, it'd be clear from the first word that you would be getting a config object back
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.
eh, I think the fact that they're coming from the queries module is clear enough. I like the short names that focus end result we've been using.
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!
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.
sorry for my way late responses, been a busy week so far!
@@ -0,0 +1,101 @@ | |||
import { QueryClient } from "@tanstack/react-query"; | |||
import * as API from "api/api"; |
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, this is what I've been doing to. it's fine. 🤷♀️
}; | ||
}; | ||
|
||
export const group = (groupId: string) => { |
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.
eh, I think the fact that they're coming from the queries module is clear enough. I like the short names that focus end result we've been using.
Related to #9598 |
No description provided.