Skip to content
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

Call API with add and remove buttons, refine loading and error strategies (#181, 191) #196

Merged
merged 21 commits into from
Jun 20, 2022

Conversation

ssciolla
Copy link
Contributor

@ssciolla ssciolla commented Jun 9, 2022

This PR aims to resolve #181 and #191.

More details

  • Separated API calling logic into an api module.
  • Added js-cookie and process for adding a CSRF token to headers.
  • Adopted react-query for handling asynchronous-related state (useQuery, useMutation).
  • Improved loading UI appearance and accessibility (Create standardized loading component or pattern #181).
  • Added API error handling and display (ErrorsDisplay).
  • Display dismiss-able alert about refreshing the page when navigation changes are first made.
  • Added eslint plugin for react-hooks.

Resources

@ssciolla ssciolla force-pushed the issue-191-add-remove-api branch from f581348 to 57d1c36 Compare June 10, 2022 18:19
body: JSON.stringify(body),
headers: {
...BASE_MUTATION_HEADERS,
'X-CSRFTOKEN': getCSRFToken() ?? ''
Copy link
Contributor Author

@ssciolla ssciolla Jun 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could maybe throw an error in getCSRFToken instead, but it seems unlikely that the CSRF token won't be there. And this would still have a clear result, the call would fail with a missing CSRF token message.

Comment on lines +10 to +17
defaultOptions: {
queries: {
retry: false,
retryOnMount: false,
staleTime: Infinity
},
mutations: { retry: false }
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

react-query includes a lot of retry and refetch behavior by default. We may want some of this eventually, but for now I thought it seemed unnecessary and easier for testing to disable it all. See https://react-query.tanstack.com/guides/important-defaults

@ssciolla ssciolla requested review from jonespm and zqian June 13, 2022 15:35
@ssciolla ssciolla marked this pull request as ready for review June 13, 2022 15:36
frontend/app/components/ErrorsDisplay.tsx Outdated Show resolved Hide resolved
frontend/app/components/Home.tsx Show resolved Hide resolved
frontend/app/api.ts Show resolved Hide resolved
@ssciolla ssciolla force-pushed the issue-191-add-remove-api branch from 7e3fea5 to 2c09eb4 Compare June 17, 2022 16:55
@ssciolla
Copy link
Contributor Author

FYI, I fixed a styling-related issue, resulting in some extra empty space on the tool cards. Should be resolved now. See b82eab2

@zqian
Copy link
Member

zqian commented Jun 20, 2022

Not sure where the 'extra empty space' was. b82eab2 looks good to me.

@ssciolla ssciolla merged commit 7c877a5 into tl-its-umich-edu:main Jun 20, 2022
@zqian zqian linked an issue Jun 20, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants