LF-4122 handle additional removal reasons in removal flow #3159

kathyavini · 2024-03-14T01:47:50Z

Description

Integration of removal functionality with inventory.

Deletion (the CREATED_IN_ERROR option) is a separate ticket and selecting it will do nothing other than return a network error.

Jira link: https://lite-farm.atlassian.net/browse/LF-4122

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Passes test case
UI components visually reviewed on desktop view
UI components visually reviewed on mobile view
Other (please explain)

Checklist:

I have commented my code, particularly in hard-to-understand areas
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
The precommit and linting ran successfully
I have added or updated language tags for text that's part of the UI
I have added "MISSING" for all new language tags to languages I don't speak
I have added the GNU General Public License to all new files

…s to RTK Query

…reason ids Previously strings defined in the component had been used as selection values

…sons-in-removal-flow

Was getting Error: A component suspended while responding to synchronous input. This will cause the UI to be replaced with a loading indicator.

kathyavini · 2024-03-14T19:25:06Z

packages/webapp/src/containers/Animals/Inventory/index.tsx

+            isOpen={removalModalOpen}
+            onClose={() => setRemovalModalOpen(false)}
+            onConfirm={handleAnimalOrBatchRemoval}
+            showSuccessMessage={false}


Success message mentions historical inventory, so I am assuming it should not be shown before that has been built out.

kathyavini · 2024-03-14T19:26:58Z

packages/webapp/src/containers/Animals/Inventory/useAnimalInventory.tsx

-      default_breed_id: animal.default_breed_id,
-    };
-  });
+  return animals


Ultimately this filter would probably be put somewhere else (once we have a view for removed animals), but for now they are simply removed from the table + KPI

SayakaOno

Working great! I like that the hook keeps Inventory so clean.
I left small comments :)

Will a snackbar or something be added later...?

packages/webapp/src/components/Animals/RemoveAnimalsModal/index.tsx

packages/webapp/src/containers/Animals/Inventory/useAnimalOrBatchRemoval.ts

packages/webapp/src/util/animal.ts

kathyavini · 2024-03-15T23:08:25Z

Thank you @SayakaOno!

Honestly I had completely forgotten that snackbar until I saw @Duncan-Brain's daily standup report today. If Duncan has work underway on that, I'll defer to his solution as I'm pretty sure it's one snackbar for both removal and deletion on Figma and I'm assuming they can be implemented pretty easily together -- @Duncan-Brain is that right?

Duncan-Brain · 2024-03-15T23:23:32Z

@kathyavini Its a bit of a challenge lol. Thinking of doing 2 snackbars or rewriting api because non atomic between animals and batch. Mostly have solution but need to troubleshoot over rendering monday.

antsgar

Works great! 🙌 I noticed two UI things that aren't really related to this PR so no need to handle them here, but just noting them. The removal reasons dropdown would look better without the padding around the options, and on the Action Menu, it always says "Remove animal" even if there's more than one selected (could be generically "Remove animals" in plural or change depending on the number selected)

antsgar · 2024-03-20T15:03:44Z

packages/webapp/src/components/Animals/RemoveAnimalsModal/index.tsx

+  // Reset form when a new set of animals/batches is selected
+  useEffect(() => {
+    if (props.isOpen) {
+      reset({ [DATE]: getLocalDateInYYYYDDMM() });
+    }
+  }, [props.isOpen]);


If we want to reset the form to default every time the modal's open, I think this can be achieved by passing shouldUnregister: true to useForm

Oh nice, thank you! I didn't know that reset to default value!

kathyavini · 2024-03-20T19:10:47Z

@antsgar let's do "Remove animals" in all cases because the singular entity selected might be a batch... and it's also easier to implement 😉

Could you show me which padding you mean? I actually specifically asked Nav to increase the padding on this dropdown beyond the default React Select 😅 -- but I was basing it on Loïc's Figma design here.

…on bar

antsgar · 2024-03-20T19:56:59Z

@kathyavini this spacing here!

Captura de pantalla 2024-03-20 a la(s) 16 53 55

I do see it in the designs too but I find it odd that the options don't go to the border of the dropdown. The top one's fine actually, it's the left right/margins that look weird to me, we could add in more spacing to the option's padding itself instead of around it?

kathyavini · 2024-03-20T20:28:28Z

@antsgar ah so that the hover effect extends like this? That can definitely be done

Alas just noticed that the extra character in Remove Animals doesn't fit 😉

Will adjust, but I imagine will we want to revisit this after all the translations are in?

…label

kathyavini · 2024-03-20T20:38:58Z

packages/webapp/src/components/ActionMenu/styles.module.scss

@@ -31,7 +31,7 @@
    left: 50%;

    &.iconCount_4 {
-      width: 408px;
+      width: 424px;


@SayakaOno this was my first thought for fitting the longer label, but now actually I think I slightly prefer the look of decreasing the gap from 8px to 4px (and boosting the width to just 416px); curious which you think is better?

I think I set the gap to 8px because the space looked too narrow with 4px.

To me, an 8 pixel gap still seems better when thinking about other languages...

SayakaOno · 2024-03-20T22:32:43Z

packages/webapp/src/components/ActionMenu/styles.module.scss

@@ -31,7 +31,7 @@
    left: 50%;

    &.iconCount_4 {
-      width: 408px;
+      width: 424px;


I think I set the gap to 8px because the space looked too narrow with 4px.

To me, an 8 pixel gap still seems better when thinking about other languages...

kathyavini added 4 commits March 13, 2024 18:37

LF-4122 Add GET of the removal reason enum and removal PATCH endpoint…

8826d9e

…s to RTK Query

LF-4122 Update component to use backend enum table values as removal …

7d80232

…reason ids Previously strings defined in the component had been used as selection values

LF-4122 Filter out removed animals and batches from inventory

9e0c3e6

LF-4122 Connect removal functionality to Inventory

8da8d89

kathyavini added the enhancement New feature or request label Mar 14, 2024

kathyavini self-assigned this Mar 14, 2024

kathyavini requested review from a team as code owners March 14, 2024 01:47

kathyavini requested review from antsgar and SayakaOno and removed request for a team March 14, 2024 01:47

kathyavini changed the base branch from integration to LF-4087/Create_or_modify_containers_to_associate_action_bar_and_table March 14, 2024 01:52

kathyavini marked this pull request as draft March 14, 2024 01:52

Base automatically changed from LF-4087/Create_or_modify_containers_to_associate_action_bar_and_table to integration March 14, 2024 16:03

kathyavini added 3 commits March 14, 2024 11:25

Merge branch 'integration' into LF-4122-handle-additional-removal-rea…

5945ce1

…sons-in-removal-flow

LF-4122 Invalidate type tags to update KPI count

13ac8e6

LF-4122 Wrap component in Storybook story in Suspense

c391e77

Was getting Error: A component suspended while responding to synchronous input. This will cause the UI to be replaced with a loading indicator.

kathyavini marked this pull request as ready for review March 14, 2024 19:23

kathyavini commented Mar 14, 2024

View reviewed changes

SayakaOno reviewed Mar 15, 2024

View reviewed changes

packages/webapp/src/components/Animals/RemoveAnimalsModal/index.tsx Show resolved Hide resolved

packages/webapp/src/containers/Animals/Inventory/useAnimalOrBatchRemoval.ts Outdated Show resolved Hide resolved

packages/webapp/src/util/animal.ts Outdated Show resolved Hide resolved

LF-4122 Fix hardcoded string values, unnecessary loading state

a8d6f62

LF-4122 Merge integration and fix conflict in Inventory container

6438a33

Duncan-Brain mentioned this pull request Mar 18, 2024

Lf 4119 display component for animal removal and handle delete reason #3165

Merged

16 tasks

kathyavini requested a review from SayakaOno March 19, 2024 16:35

LF-4122 Merge integration and fix conflict in useAnimalInventory hook

c7efa33

SayakaOno previously approved these changes Mar 19, 2024

View reviewed changes

antsgar previously approved these changes Mar 20, 2024

View reviewed changes

LF-4122 Unregister instead of manually resetting form; pluralize acti…

3534fff

…on bar

kathyavini dismissed stale reviews from antsgar and SayakaOno via 3534fff March 20, 2024 19:21

kathyavini added 2 commits March 20, 2024 13:31

LF-4122 Move menuList padding into option

31f637c

LF-4122 Give action bar slightly more width to accomodate pluralized …

659e83c

…label

kathyavini commented Mar 20, 2024

View reviewed changes

SayakaOno approved these changes Mar 20, 2024

View reviewed changes

kathyavini requested a review from antsgar March 20, 2024 22:51

antsgar approved these changes Mar 21, 2024

View reviewed changes

antsgar added this pull request to the merge queue Mar 21, 2024

Merged via the queue into integration with commit 26e6273 Mar 21, 2024
5 checks passed

antsgar deleted the LF-4122-handle-additional-removal-reasons-in-removal-flow branch March 21, 2024 16:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LF-4122 handle additional removal reasons in removal flow #3159

LF-4122 handle additional removal reasons in removal flow #3159

kathyavini commented Mar 14, 2024 •

edited

Loading

kathyavini Mar 14, 2024

kathyavini Mar 14, 2024

SayakaOno left a comment •

edited

Loading

kathyavini commented Mar 15, 2024

Duncan-Brain commented Mar 15, 2024

antsgar left a comment

antsgar Mar 20, 2024

kathyavini Mar 20, 2024

kathyavini commented Mar 20, 2024

antsgar commented Mar 20, 2024

kathyavini commented Mar 20, 2024

kathyavini Mar 20, 2024 •

edited

Loading

SayakaOno Mar 20, 2024

SayakaOno Mar 20, 2024

LF-4122 handle additional removal reasons in removal flow #3159

LF-4122 handle additional removal reasons in removal flow #3159

Conversation

kathyavini commented Mar 14, 2024 • edited Loading

kathyavini Mar 14, 2024

Choose a reason for hiding this comment

kathyavini Mar 14, 2024

Choose a reason for hiding this comment

SayakaOno left a comment • edited Loading

Choose a reason for hiding this comment

kathyavini commented Mar 15, 2024

Duncan-Brain commented Mar 15, 2024

antsgar left a comment

Choose a reason for hiding this comment

antsgar Mar 20, 2024

Choose a reason for hiding this comment

kathyavini Mar 20, 2024

Choose a reason for hiding this comment

kathyavini commented Mar 20, 2024

antsgar commented Mar 20, 2024

kathyavini commented Mar 20, 2024

kathyavini Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

SayakaOno Mar 20, 2024

Choose a reason for hiding this comment

SayakaOno Mar 20, 2024

Choose a reason for hiding this comment

kathyavini commented Mar 14, 2024 •

edited

Loading

SayakaOno left a comment •

edited

Loading

kathyavini Mar 20, 2024 •

edited

Loading