-
Notifications
You must be signed in to change notification settings - Fork 39
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
HUM-99: available jobs service refactor #3032
HUM-99: available jobs service refactor #3032
Conversation
Someone is attempting to deploy a commit to the HUMAN Protocol Team on Vercel. A member of the Team first needs to authorize it. |
@dnechay ready for review |
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.
New "fetch-available-jobs" service is really a "service" now, because it has no react-specific logic, just API-related stuff.
However, I don't think it makes sense to split it to service, schema and types file, it all can live in a single file. Later if we come up to some kind of jobService
(that will hold all job-related API calls) - it can make sense, but right now we just producing more & more "nano" files that doesn't make a lot of sense. So let's keep things together for now
packages/apps/human-app/frontend/src/modules/worker/hooks/use-available-jobs-query.ts
Outdated
Show resolved
Hide resolved
...end/src/modules/worker/components/jobs/available-jobs/mobile/available-jobs-table-mobile.tsx
Show resolved
Hide resolved
.../human-app/frontend/src/modules/worker/services/fetch-available-jobs/fetch-available-jobs.ts
Outdated
Show resolved
Hide resolved
...app/frontend/src/modules/worker/services/fetch-available-jobs/fetch-available-jobs.schema.ts
Outdated
Show resolved
Hide resolved
5fdc61a
to
8602c56
Compare
@dnechay this one is ready as well. I think further changes should be commited here as adding more here could cause unnecessary conflicts |
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.
LGTM overall
One question: do we need to export availableJobSchema
and availableJobsSuccessResponseSchema
, @mpblocky ? They don't seem to be used anywhere else
@dnechay You are right, it was exported before and forgot about it while removing it's duplicates. Fixed! |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@dnechay can You merge this PR? It blocks me a bit from further work |
Issue tracking
HUM-99
Context behind the change
Split file into hooks, schema, types
How has this been tested?
Release plan
normal deploy
Potential risks; What to monitor; Rollback plan
N/A