Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

#172 - refactor request table #233

Merged
merged 9 commits into from
Nov 14, 2022

Conversation

obr42
Copy link
Contributor

@obr42 obr42 commented Nov 10, 2022

Closes #172
Closes #96

Refactored request page to use the main table component. Was able to remove the file completely, as well as the Cache Service.

@knolnsokn
Copy link
Contributor

Conflicts with main

@obr42
Copy link
Contributor Author

obr42 commented Nov 12, 2022

Conflicts with main

Its still in draft - not quite done yet :) I'll make sure they are taken care of before I move it out of draft

@obr42 obr42 requested a review from a team November 12, 2022 02:59
@obr42 obr42 marked this pull request as ready for review November 12, 2022 02:59
@@ -146,7 +146,7 @@ const SyncUserModal = ({ open, setOpen }: ModalProps) => {
.catch((e) => {
setAlert({
severity: 'error',
message: e,
message: e.response.data.message,
Copy link
Contributor

@jlrpnbbngtn jlrpnbbngtn Nov 14, 2022

Choose a reason for hiding this comment

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

response is optional in AxiosError, so maybe something like

message: e.response?.data.message ?? e.toJSON()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is e.response.data.message || e everywhere else I just overlooked it for this PR for some reason 🙃

@@ -11,17 +11,17 @@ const useJobs = () => {
const { axiosManualOptions } = useMyAxios()
const [, execute] = useAxios({}, axiosManualOptions)

const getJobs = () => {
const config: AxiosRequestConfig<Job[]> = {
const getJobs = (): Promise<AxiosResponse<Job[]>> => {
Copy link
Contributor

@jlrpnbbngtn jlrpnbbngtn Nov 14, 2022

Choose a reason for hiding this comment

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

The parameterization of the AxiosRequestConfig was not wrong, but it bought us nothing. I like this a lot better, though if we wanted to be pedantic we could call this AxiosPromise<Job[]>, which also buys us nothing over what you have here ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I think that adding typing to AxiosRequestConfig actually types what is going to be in data in the options, which those two particular calls don't have any data so that is why I removed it. I had tried putting <Job> on the config for one of the other calls - maybe resume? - that returns a Job but accepts not a Job for data, and TypeScript errored on the typing. I didn't know there was an AxiosPromise type I had only seen the response type I will change it to the promise that looks a bit cleaner

@jlrpnbbngtn jlrpnbbngtn merged commit b8d422d into beer-garden:main Nov 14, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request scheduler table refactor Refactor out CacheService
3 participants