-
Notifications
You must be signed in to change notification settings - Fork 1
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
Split up useFilteredConsultants to smaller hooks #265
Conversation
Co-authored-by: Sigrid Elnan <[email protected]>
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.
For det første, sykt bra jobba, koden ble mye mer leselig nå, og ser mye enklere ut å jobbe med! 🔥
Men jeg får ikke "denne uka" knappen til å funke.
Punktum havner i søkefeltet, dette er kanskje ikke ideelt? 🤔
|
||
return ( | ||
<div className="flex flex-row gap-1"> | ||
<SecondaryButton label={"Denne uka"} onClick={resetSelectedWeek} /> | ||
<IconSecondaryButton | ||
icon={<ArrowLeft size={24} />} | ||
onClick={decrementSelectedWeek} | ||
onClick={() => changeSelectedWeek(-7)} |
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.
As the number of weeks changed is based on number of weeks shown, this needs to use -7, -11 or -23. Same with the function below 😅
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.
Good catch! 🎉 This will be changed based on the new weekSpan
variable coming with the PR from @mathildehaugum :)
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.
Nice!
} | ||
|
||
function incrementSelectedWeek() { | ||
changeSelectedWeek(6); |
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.
Should this use weekSpan - 1? 😄
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.
Nice catch. Is fixed now.
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.
Really well done! I have one final comment, but it´s not essential 😅
const { numWeeks = weekSpan } = updateParams; | ||
const { availability = availabilityFilter } = updateParams; | ||
|
||
console.log("Update route with numWeeks = " + numWeeks); |
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.
Maybe remove this? 😄
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.
done 👍
Basically refactor the useFilteredConsultants into separate hook.
NB: This is an initial refactor, and will improve further.