Lf 4483 create selected animals summary component

[Duncan-Brain]

Description

This creates the animal summary component -- found out yesterday Loic considers this a nice to have. But it is complete now so 🤷 It seems back in scope for read-only + completion views.

Notes:

• There is a google maps crash on timeout + expanding item -- based on a previous encounter I think it will resolve in production as relating to react strict mode and the fact our google maps package is maybe out of maintenance - Nav menu styling fixes #3059 (comment)
• Things I am not super comfortable with about the solution:
  • a) re-using the AnimalInventory container and making variants (views) of it, I think containers are meant to be single use for the most part so I might have preferred to split this up more.
  • b) double use of useAnimalInventory() -- one for ExpandableAnimalInventory to calculate animalCount and one for regular animal inventory use. I tried to conditionally render the expandable in a single component but the container height got twitchy when selecting an animal on previous animal selection screen.

Jira link: LF-4483

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?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

• 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

[antsgar]

@Duncan-Brain when you branch off of another feature branch, my suggestion is that you set that branch as target for your PR. That way it'll show the right diff and it can be reviewed even if the other PR hasn't been merged yet. Once the other PR is merged, GH will automatically change the target branch to integration for you 🙌

[antsgar]

Left a few suggestions re code structure but this is looking amazing, I love it!

[antsgar]

I think there's a cleaner way to do this both for container and component that would prevent having to add more combinations of these parameters as we go along (and also a more "React-y" way of doing things, if that term makes any sense 😅), would be to actually make these configurable parameters props for the component, and set a default for them. So everything in the "default" block here would be the default for these props, and then each parent component would have the power to override them to something different if needed. That way we'd just be making the component more flexible, instead of pre-defining a set of uses for it.

If we still thought it was useful to have a defined set of use cases for the component, we could then go ahead and define them as parent components. Something like this:

const AnimalsInventorySummary = () => {
  <AnimalsInventory
    tableMaxHeight={...}
    tableSpacerRowHeight={...}
    ...
  />
}

const AnimalsInventoryTaskSelection = () => {
  <AnimalsInventory
    tableMaxHeight={...}
    tableSpacerRowHeight={...}
    ...
   />
}

[Duncan-Brain]

Yeah I hated this part of it the solution. Good call out. I will do that first suggestion.

My instinct was to make more containers and bundle the reused logic into a hook -- but it seemed liek it would take too long. But ideally we would reuse components and not containers. That would make components like KPI and FloatingActionBar not even be possible for task animals Inventory.

[Duncan-Brain]

So I am still not happy with the solution, but I think it is at least better if you could check it out.

[antsgar]

Doesn't this break the rules of hooks since you're calling a hook within a conditional? https://react.dev/reference/rules/rules-of-hooks#only-call-hooks-at-the-top-level

I think we could just add a condition to animalOrBatchMatches like so

const animalOrBatchMatches =
        showOnlySelected && selectedInventoryIds.includes(entity.id) ||
        isInactive(animalsOrBatchesFilter) ||
        (entity.batch
          ? animalsOrBatchesFilter[AnimalOrBatchKeys.BATCH]?.active
          : animalsOrBatchesFilter[AnimalOrBatchKeys.ANIMAL]?.active);

[Duncan-Brain]

Wow great point about breaking the hook rules that was silly of me. I dont know why I didn't get a warning either. Will do that!

[Duncan-Brain]

Tried it the suggested way, but the other matches returned extra results which was not desired.

Because selectedIds is likely to change more frequently I just moved it out of the conditional but kept its own separate useMemo.

[antsgar]

Following up on the idea I mentioned above of having different parent components for the different "versions" of the inventory, we could set inventory as a prop for the AnimalInventory container, and then call useInventory from the parent components. So we'd end up with something like:

const AnimalsInventorySummary = () => {
  const { inventory, isLoading } = useAnimalInventory();
  const animalCount = ...;
  <ExpandableAnimalInventory
    ...
    expandedContent={
      <AnimalsInventory
        inventory={inventory}
        isLoading={isLoading}
        tableMaxHeight={...}
        tableSpacerRowHeight={...}
        ...
       />
    }
  />
}

const AnimalsInventoryTaskSelection = () => {
  const { inventory, isLoading } = useAnimalInventory();
  <AnimalsInventory
    inventory={inventory}
    isLoading={isLoading}
    tableMaxHeight={...}
    tableSpacerRowHeight={...}
    ...
   />
}

so the double call of useAnimalInventory would also be solved

[Duncan-Brain]

Let me know what you think of the newer refactor... its still not optimal. Theres just so much shared logic I found it hard to split away without making it look more complex.

[kathyavini]

I think this is in progress based on the comments thread, and I'm not done reading the files either, but just wanted to leave a few small notes! It looks good!

[kathyavini]

I hate to comment because I know it will have to go back around to Loïc, but... it's not grammatical, is it? Shouldn't it be "detailed list"?

[Duncan-Brain]

I think a list of details could be a "detail list" where "detail list" is the object of the sentence. "See a detailed list.. " makes just "list" the object of the sentence with detailed describing it. Its not common but not wrong either I don't think.

[Duncan-Brain]

So I am still not super happy with the size of the AnimalInventory container -- it almost feels like I would rather duplicate code than have it be used for all these cases... But it works and I think it somewhat resembles TaskLocations style component switching.

[kathyavini]

The component looks totally 🔥 (well except for the mobile header -- which will get fixed 😂 ) and it's hard to imagine how many CSS problems you had to solve to get this working! Personally I like how the Inventory variants are set up now -- agree its so much code in <AnimalInventory /> but I think each variant reads clearly. And they are definitely easy to consume, having tested them in the task readonly + complete 👍

Only thing which I thought needs a tweak is the useFilteredInventory logic (the other one is optional since I can add it on my PR).

[antsgar]

The refactor looks awesome, much better than before IMO! Excellent work 🔥 I'd take it one step further by letting the "variant" components own passing to BaseAnimalInventory the props that are specific to each variant and not common to all of them. I'm not sure if I'm explaining it clearly, so I pushed out an alternative branch showcasing this LF-4483-create-selected-animals-summary-component-2 (I haven't tested it thoroughly though so please do if you decide to move forward with that change)

[kathyavini]

Sorry I think I missed something -- it might be what you referred to here

here is a google maps crash on timeout + expanding item

but I don't think so, because it is happening on the screens that don't include the expandable, and timeout doesn't seem to be required. Actually I'm not even sure what the error is because my console is only showing warnings but there is a full whitescreen on the location selecting step of tasks OTHER than movement, could you check if you see that?

(I was doing my custom tasks investigation when I ran into it, but I'm seeing this crash on all tasks except movement).

[Duncan-Brain]

@antsgar I merged your branch into mine -- seems good, thanks!

@kathyavini I was using logic for styles like it was className but doesnt work for style or classes obviously -- should be fixed in latest "merge commit" I got mixed up between branches again and committed to my local of Antos improvement.

[kathyavini]

I was using logic for styles like it was className but doesnt work for style or classes obviously

I actually got confused in the exact same way looking at the console from the crash! I saw the console warning related to the styling, but thought "that's not the reason for the crash; classnames can't crash" 😂 But I missed that it was a style object!

Looks great to me!