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.

[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!

[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

[antsgar]

This is totally okay but I think I personally would find this a bit more readable

const animalCount = inventory.find((animalOrBatch) => props.preSelectedIds.includes(animalOrBatch.id)).length;

[Duncan-Brain]

Its got to be count not length because of batches so I don't think I can do this.

[antsgar]

Ah!! I totally missed the count part, makes sense ☺️

[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.

[kathyavini]

Oh wow I wasn't a fan of the pill having to be a prop to ExpandableItem instead of passed in mainContent, but I guess the way Loïc designed the mobile view (with the collapse icon only next to the text) requires it?? A bit of a gnarly layout challenge you solved there!

Would it be hard to update the Storybook Story for ExpandableItem to cover/illustrate this new pill case?

[kathyavini]

The mobile layout should extend to 1200px? Or should @include here and in .alwaysVisibleContent be sm-breakpoint?