-
Notifications
You must be signed in to change notification settings - Fork 78
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
[Not urgent!] LF-4384(b): Refactor useAnimalOrBatchRemoval #3549
base: integration
Are you sure you want to change the base?
Conversation
…d SingleAnimalView
984e09a
to
5290430
Compare
setSelectedInventoryIds((selectedInventoryIds) => { | ||
return selectedInventoryIds.filter((i) => { | ||
const { kind, id } = parseInventoryId(i); | ||
return kind === animalOrBatchKey && ids.includes(id); |
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.
The previous filter was : !selectedAnimalIds.includes(i))
Everything looks like it works so I am not sure it matters, but it looks like it is setting the selected inventory id to the ones that were successfully removed?
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.
Thank you so much for catching it! I remember getting confused about how to use .filter()
suddenly and ended up flipping the condition! 🙏
It's 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.
Left an inline comment but I am not sure it is valid.
Description
Currently the
useAnimalOrBatchRemoval
hook accepts inventory IDs and parses them. This parsing is redundant for theSingleAnimalView
, which already has a parsed ID. This PR makes the hook more generic by:useAnimalOrBatchRemoval
to accept:id
s withkind
(animal or batch key) instead of inventory IDsonSuccess
instead ofsetSelectedInventoryIds
setSelectedInventoryIds
logic to theInventory
component.(This PR addresses the comment on #3534)