-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Cases] refactor useGetCases to use react-query #133505
[Cases] refactor useGetCases to use react-query #133505
Conversation
…nto refactor/create-form-hooks-part3
…nto refactor/create-form-hooks-part3
Pinging @elastic/response-ops (Team:ResponseOps) |
Pinging @elastic/response-ops-cases (Feature:Cases) |
dispatchUpdateCaseProperty({ | ||
...args, | ||
refetchCasesStatus: () => { | ||
({ updateKey, updateValue, caseData }: UpdateByKey) => { |
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 one of the main logic-wise changes in this PR. The previous useGetCases hook was providing a "sub-hook" to update a case and I had to remove that.
Instead I reused the existing updateCaseproperty hook.
@@ -65,19 +72,21 @@ export const AllCasesList = React.memo<AllCasesListProps>( | |||
...(!isEmpty(hiddenStatuses) && firstAvailableStatus && { status: firstAvailableStatus }), | |||
owner: hasOwner ? owner : availableSolutions, | |||
}; | |||
const [filterOptions, setFilterOptions] = useState<FilterOptions>({ |
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 the second logic change in the PR. the previous hook was working both as a fetch data hook and state management. I removed the state management part from it and move it here.
@@ -147,30 +156,28 @@ export const AllCasesList = React.memo<AllCasesListProps>( | |||
const onFilterChangedCallback = useCallback( | |||
(newFilterOptions: Partial<FilterOptions>) => { | |||
if (newFilterOptions.status && newFilterOptions.status === CaseStatuses.closed) { | |||
setQueryParams({ sortField: SortFieldCase.closedAt }); | |||
setQueryParams({ ...DEFAULT_QUERY_PARAMS, sortField: SortFieldCase.closedAt }); |
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.
Shouldn't we use the previous state?
setQueryParams((prev) => ({ ...prev, sortField: SortFieldCase.closedAt }))
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.
Correct! thanks :D
@@ -240,8 +246,8 @@ export const AllCasesList = React.memo<AllCasesListProps>( | |||
filterOptions={filterOptions} | |||
goToCreateCase={onRowClick} | |||
handleIsLoading={handleIsLoading} | |||
isCasesLoading={isCasesLoading} | |||
isCommentUpdating={isCasesLoading} | |||
isCasesLoading={isLoadingCases && data.cases.length === 0} |
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.
What will happen if the are no cases in the response?
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 actually a mistake. I removed the length check.
💚 Build SucceededMetrics [docs]Async chunks
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @academo |
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.
Code LTGM. Tested locally without any issues.
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.
Tested and LGTM
Summary
Refactors de useGetCases hook to use react-query.
This refactor changes many other components that use this hook.