-
Notifications
You must be signed in to change notification settings - Fork 0
[Full-Stack] Positions Cleanup #143
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
76fa914 to
f4d639e
Compare
| @@ -0,0 +1,66 @@ | |||
| import { type AssessmentWithRelations } from '@/lib/services/assessment.service'; | |||
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 think it'd be good to define the types within the types directory - thoughts?
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.
Sure thing. Will fix, thanks
| @@ -0,0 +1,48 @@ | |||
| import { | |||
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 see this is related to abstracting away the fetch calls from hooks can u explain the reasoning for doing it this way
|
|
||
| <TabsContent value="active" className="flex flex-col gap-4"> | ||
| {positions.length > 0 ? ( | ||
| {active.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.
when we say active are we referring to the active tab selected or the positions that are active / live
|
|
||
| handleCreate, | ||
| handleCancel, | ||
| } = useCreateCandidateModal({ |
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.
https://www.reddit.com/r/reactjs/comments/xwzptl/whats_your_opinion_on_extracting_all_or_most/
I see we're extracting logic for state and effect to these hooks - ig this is up to us but is this common?
I think its clean but also might be overabstracting things that aren't going to be used elsewhere
ig in the future if we decide to have a create candidate modal functionality elsewhere then this might be helpful
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 we can move away from actions if its something people aren't a hundo on
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.
we can do similar things with tanstack query I believe
| export interface CreatePositionModalProps { | ||
| open: boolean; | ||
| onOpenChange: (open: boolean) => void; | ||
| setActive: React.Dispatch<React.SetStateAction<PositionWithCounts[]>>; |
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.
did we move away from useState to use the dispatch? If we did, what is the reasoning (not familiar with dispatch)
| @@ -0,0 +1,29 @@ | |||
| import type { PositionPreviewData } from './usePositionPreviewModal'; | |||
|
|
|||
| export type Candidate = NonNullable<PositionPreviewData>['candidates'][number]; | |||
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.
Why do we have to define it as NonNullable? Were there typing issues otherwise?
| toast.error('Batch creation failed'); | ||
| } catch (err) { | ||
| const message = err instanceof Error ? err.message : 'Batch creation failed'; | ||
| console.error(err); |
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.
we should remove this console error before we merge :)
| setError('Failed to batch create candidates'); | ||
| toast.error('Batch creation failed'); | ||
| } catch (err) { | ||
| const message = err instanceof Error ? err.message : 'Batch creation failed'; |
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.
why do we need to check the instance I'm assuming its because you do err.message so we need to check the type of the err object right?
|
|
||
| // In the future we may want to add the ability to create a position AND upload a CSV | ||
| // of candidates at the same time. If that were the case, we'd want to create a new | ||
| // route to create a position with the count fields included in the returned object |
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.
In this case we could have it create the position and then update it - so it'd be two calls rather than a single create
we would then be using previous logic
thoughts?
[Full-Stack] - Positions Cleanup
Changes
fetchcalls in hooks tolib/api/*.GET /api/positions?orgId=...positions/action.tsand replaced it with endpoints in/api/positions/route.tsScreenshots
Checklist
Please go through all items before requesting reviewers:
Closes
Closes #127