Lf 4083 add search to animal list #3152

Duncan-Brain · 2024-03-07T23:50:47Z

Description

Adds search bar to Inventory page.

Does not:

add stickyness as discussed on ticket and now handled by LF-4142 Adjust inventory layout
Change where the Layout component is from component to container and the search bar utilities. @kathyavini is it right I thought I heard you mention you wanted it elsewhere but told me not to change it yet -- are you wanting to tackle that in LF-4104?

Jira link: LF-4083

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

…h-to-animal-list

kathyavini · 2024-03-08T18:21:00Z

@Duncan-Brain no I think the idea of moving it was nixed altogether, since it was about maintaining the same structure as transactions (and keeping the container/component distinction) which were both deemed arbitrary. If we aren't worried about either of those two points there is no need to move where the data gets manipulated (there will some more passing of callbacks maybe but it's doable).

kathyavini · 2024-03-08T21:03:59Z

So did you and Loïc decide not to implement the modal effect when clicking into the searchbar on mobile? That's what all that resize observer code and searchOverlayOpen code in <PureCollapsibleSearch /> is about (it's not related to collapse).

I actually don't mind not having the effect (and wouldn't mind removing it for transactions, as we should be consistent and the need for resize observer is kind of annoying -- you can see how all that logic bulked up that component file), but if you aren't going for the modal effect then is no need to use <PureCollapsibleSearch /> as your base component -- you can just use regular <PureSearchbarAndFilter />. The way you wrapped all the additional logic in collapse you kind of re-generated the base searchbar anyway 😉

Personally I vote for removing the modal effect in both places -- I mean after discussing with Loïc of course!! -- but I'm curious what the team thinks.

Duncan-Brain · 2024-03-08T21:11:25Z

@kathyavini Good question, I think I missed that screen haha.

I agree I don't need it personally but I will make it the same for now!

kathyavini · 2024-03-08T22:04:47Z

@Duncan-Brain I dunno; it's kind of a mess as written, isn't it? It's my component so I take responsibility 😅 I'm thinking of trying to re-write it so the modal functionality and the collapsing functionality reside in separate components; I assumed when I made it that this was going to be a new pattern for all searches so I made those two things too dependent on each other.

Edit: Or maybe that's not possible because the modal IS the collapsed state's searchbar 😭 Messy. Probably a fresh component for modal functionality would be easier.

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

Duncan-Brain · 2024-03-11T22:16:18Z

Took @kathyavini advice and just copied and cut the non desired pieces for refactoring another day!

packages/webapp/public/locales/en/translation.json

packages/webapp/src/components/PopupFilter/PureCollapsibleSearch/styles.module.scss

packages/webapp/src/components/PopupFilter/PureSearchWithBackdrop/index.jsx

packages/webapp/src/components/PopupFilter/PureSearchWithBackdrop/styles.module.scss

packages/webapp/src/components/PopupFilter/PureSearchWithBackdrop/index.jsx

kathyavini

Wait by using <Backdrop /> you were able to skip needing a resize observer???! 😍 Okay I want that in the old component now so much!!!

There is just one range of screen widths (tablet range) where everything goes insane -- is this a byproduct of <Backdrop /> or unrelated?

packages/webapp/src/components/PopupFilter/PureCollapsibleSearch/index.jsx

Duncan-Brain · 2024-03-12T14:17:04Z

@kathyavini Yeah thanks I def missed that weird behaviour. It's because of the Table I didn't update that to the mobile behaviour in line with the designs -- fixed now.

I will remove the collapse and update stories but then rename it from Collapsible to Collapsing so as not to be misleading.

But it raised some interesting issues there that I will ignore for now haha, but collapsible icon does not seem to be responsive to the menu, and layout should probably overflow right creating a scrollbar instead of forcing the flex state of the menu.

packages/webapp/src/stories/PureSearchBarWithBackdrop/PureSearchBarWithBackdrop.stories.jsx

SayakaOno

It's working well!
I'm wondering:

whether we should use the isDesktop prop rather than breakpoints in scss?
switching with or without the table header at 1200px seems a bit wide to me. (but I understand < 1200px is considered as tablet size) Can we find the requirement in Figma? (this is a general question...)

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

SayakaOno · 2024-03-12T18:20:54Z

packages/webapp/src/components/PopupFilter/PureCollapsingSearch/index.jsx

  value,
  isSearchActive,
  onChange,
  placeholderText,
  className,
  containerRef,
+  isDesktop,


We shouldn't use the breakpoint?

It was a media query -- no we should not use media query I don't think.

mixin.scss has breakpoints that are OK since we can change the whole app at the same time. If this is the preferred choice I can change 🤷 .

isDesktop is nice in my personal opinion because:

we copied MUI breakpoints to make the mixin.scss so now we have 2 copies of source of truth

Using good CSS class naming like BEM I think javascript files can be slightly more readable versus hidden in CSS (I thought this was blocker so I rushed naming but fixed to BEM)

sometimes we want to use isDesktop for in page content or design decisions too so I enjoy letting MUI control our design system as a whole (since they are the experts)

I really like having the breakpoints being controlled by JS/TS and no longer the mixins... those multiple methods and sources of truth (when finances was created we weren't yet unified around the MUI breakpoints) made it really hard to reason about e.g. the CollapsibleSearch.

But GOSH I hate those BEM underscores... do we have to?? 🤣

Edit: and more in the spirit of logic and not just knee-jerk underscore dislike, I don't think we want to adopt a new CSS naming convention like this without discussion + deciding if it's something we'll use moving forward; a mix of naming conventions is pretty confusing.

lol no not necessary -- but its a good architecture vs "willy nilly" haha

Okay, I was in favour of using CSS. I don't think we have discussed which strategy to stick with? I added to the tech daily agenda!

@SayakaOno sounds awesome! I don't have a strong opinion between mixins and inline breakpoints just no more media queries haha

I don't think we made a decision today. @SayakaOno can I make a tech debt ticket to choose ? That way search bar can go through.

I thought we wouldn't need any further discussion 😅 It's up to you!

I think it would be nice to choose a direction haha: https://lite-farm.atlassian.net/browse/LF-4143

packages/webapp/src/stories/PureSearchBarWithBackdrop/PureSearchBarWithBackdrop.stories.jsx

antsgar

Left a couple of comments re the CSS naming conventions! Other than that it works great, excellent work 🙌

antsgar · 2024-03-13T20:19:16Z

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

-      />
+      <div
+        className={clsx(
+          isDesktop ? styles.SearchAndFilter_container___desktop : styles.SearchAndFilter_container,


I like BEM overall, but since we haven't made a decision to go for that throughout the app, I'd advocate for consistency here and naming the styles more similarly to how classes are currently named in other files. In general I'm all for BEM but I think these will be out of place unless there's overall consensus to go with BEM from now on. If we do make a decision to go with BEM from now on, I think the convention would actually be something like search-and-filter__container--desktop. Unless this is a different one and not BEM?

I think there are some react problems with the - or something. I linked the article I used to try to apply it but it may be out of date.

I just added it to discuss the isDesktop inline breakpoints readability.

antsgar · 2024-03-13T20:23:47Z

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

+        />
+        <div
+          className={clsx(
+            isDesktop ? styles.SearchResults___marginLeft8 : styles.SearchResults___textAlignCenter,


I'd try to give these classes more generic names rather than names that specifically point out the styling applied. The reason for that is, it leaves little flexibility to update the styling and reuse that selector without having to change the name of the class. Even if we went for BEM, the "modifiers" used are usually things like "compact", "disabled", "active", "big" rather than calling out specific styles

Yeah. I would only call out specific styles if it is the only style on the property. I don't see value in making the modifier less readable (in JS) when there is just one style.

But yeah anyone wanting to add another style would probably rename it to be more generic. Most just showcasing the inline breakpoints readability -- I haven't mastered BEM by any means ahha so what your seeing is my interpretation .. but BEM is nice overall

Duncan-Brain · 2024-03-13T20:56:56Z

@antsgar BEM is gone!

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

SayakaOno

🙌

SayakaOno · 2024-03-13T21:33:59Z

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

+      {hasSearchResults ? (
+        <Table
+          kind={TableKind.V2}
+          alternatingRowColor={true}
+          columns={animalsColumns}
+          data={filteredInventory}
+          shouldFixTableLayout={isDesktop}
+          minRows={filteredInventory.length}
+          dense={false}
+          showHeader={isDesktop}
+        />
+      ) : (
+        <NoSearchResults
+          className={clsx(isDesktop ? styles.noSearchResultsDesktop : styles.noSearchResults)}
+          searchTerm={searchString}
+          includeFiltersInClearSuggestion
+        />
+      )}


I looked into "+ Load more" button being shown sometimes.
In Table, minRows is the initial value of data.length and won't change.

In Inventory component, if you have 10 animals and filter out 6 animals and refresh the page, minRows is 4 and when you clear filters, "Load more" button shows up.

The same thing happens after hasSearchResults is changed to false to true here since Table is re-rendered.

(I hope it makes sense)

I will make a change to pass inventory.length as minRows from the container in my PR later.

Nice! Right that makes sense I think it doesn't need to be dynamic unless data is refetched.

Duncan-Brain added 5 commits March 6, 2024 09:32

LF-4083 - Add opt-out for collapse behaviour of search

dcd2a5c

LF-4083 - Add search to Animal Inventory , fix useSearchFilter jsDoc

d228053

Merge branch 'LF-4103-implement-animals-table' into LF-4083-add-searc…

99e8508

…h-to-animal-list

Merge branch 'integration' into LF-4083-add-search-to-animal-list

aeca840

LF-4083 - Fix a few styling issues

bb11c74

Duncan-Brain requested review from a team as code owners March 7, 2024 23:50

Duncan-Brain requested review from antsgar and SayakaOno and removed request for a team March 7, 2024 23:50

Duncan-Brain self-assigned this Mar 8, 2024

SayakaOno reviewed Mar 8, 2024

View reviewed changes

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

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

Duncan-Brain added 3 commits March 11, 2024 16:03

LF-4083 - add translations, move search hooks to container

ab76695

LF-4083 - make new searchbar type

311b2a3

LF-4083 - remove excess styles

3be636f

Duncan-Brain requested a review from SayakaOno March 11, 2024 22:16

SayakaOno reviewed Mar 11, 2024

View reviewed changes

kathyavini reviewed Mar 12, 2024

View reviewed changes

packages/webapp/src/components/PopupFilter/PureCollapsibleSearch/index.jsx Outdated Show resolved Hide resolved

LF-4083 - fix strings and copyright

f0b16ca

Duncan-Brain added 4 commits March 12, 2024 10:18

LF-4083 - fix weird tablet behaviour, conditional zIndexing

1fbe4a6

LF-4083 - Remove collapse from old component, and update stories

68564b7

LF-4083 rename search filter

dd75c2a

LF-4083 - update naming

af075d7

Duncan-Brain added 3 commits March 12, 2024 12:07

LF-4083 - small removals and fixes

10ac382

LF-4083 - remove unused imports, correct conditionals

f783e27

LF-4083 - remove unused style

4a79dbf

Duncan-Brain requested review from SayakaOno and kathyavini March 12, 2024 16:16

SayakaOno reviewed Mar 12, 2024

View reviewed changes

packages/webapp/src/stories/PureSearchBarWithBackdrop/PureSearchBarWithBackdrop.stories.jsx Outdated Show resolved Hide resolved

LF-4083 - use correct i18n syntax and copyright

24af1bd

Duncan-Brain requested a review from SayakaOno March 12, 2024 18:11

SayakaOno reviewed Mar 12, 2024

View reviewed changes

Duncan-Brain added 2 commits March 12, 2024 16:29

LF-4083 - update styles naming, condense code

403bfdc

LF-4083 - remove ununsed type

4891cd4

Duncan-Brain requested a review from SayakaOno March 12, 2024 20:43

Duncan-Brain added 2 commits March 13, 2024 15:23

Merge branch 'integration' into LF-4083-add-search-to-animal-list

4273ef4

LF-4083 - fix copyright

3d399ca

antsgar reviewed Mar 13, 2024

View reviewed changes

LF-4083 - remove inconsistent styles

8c781fb

Duncan-Brain requested a review from antsgar March 13, 2024 20:57

SayakaOno reviewed Mar 13, 2024

View reviewed changes

packages/webapp/src/containers/Animals/Inventory/index.tsx Outdated Show resolved Hide resolved

LF-4083 - missed merge conflict repurcussion

62bb1a7

SayakaOno approved these changes Mar 13, 2024

View reviewed changes

antsgar approved these changes Mar 13, 2024

View reviewed changes

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

Merged via the queue into integration with commit bc41988 Mar 13, 2024
5 checks passed

antsgar deleted the LF-4083-add-search-to-animal-list branch March 13, 2024 21:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lf 4083 add search to animal list #3152

Lf 4083 add search to animal list #3152

Duncan-Brain commented Mar 7, 2024 •

edited

Loading

kathyavini commented Mar 8, 2024 •

edited

Loading

kathyavini commented Mar 8, 2024 •

edited

Loading

Duncan-Brain commented Mar 8, 2024

kathyavini commented Mar 8, 2024 •

edited

Loading

Duncan-Brain commented Mar 11, 2024

kathyavini left a comment

Duncan-Brain commented Mar 12, 2024

SayakaOno left a comment

SayakaOno Mar 12, 2024

Duncan-Brain Mar 12, 2024

kathyavini Mar 12, 2024 •

edited

Loading

Duncan-Brain Mar 12, 2024

SayakaOno Mar 12, 2024

Duncan-Brain Mar 12, 2024

Duncan-Brain Mar 13, 2024

SayakaOno Mar 13, 2024

Duncan-Brain Mar 13, 2024

antsgar left a comment

antsgar Mar 13, 2024

Duncan-Brain Mar 13, 2024

antsgar Mar 13, 2024

Duncan-Brain Mar 13, 2024 •

edited

Loading

Duncan-Brain commented Mar 13, 2024

SayakaOno left a comment

SayakaOno Mar 13, 2024

Duncan-Brain Mar 13, 2024

Lf 4083 add search to animal list #3152

Lf 4083 add search to animal list #3152

Conversation

Duncan-Brain commented Mar 7, 2024 • edited Loading

kathyavini commented Mar 8, 2024 • edited Loading

kathyavini commented Mar 8, 2024 • edited Loading

Duncan-Brain commented Mar 8, 2024

kathyavini commented Mar 8, 2024 • edited Loading

Duncan-Brain commented Mar 11, 2024

kathyavini left a comment

Choose a reason for hiding this comment

Duncan-Brain commented Mar 12, 2024

SayakaOno left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kathyavini Mar 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antsgar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Duncan-Brain Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

Duncan-Brain commented Mar 13, 2024

SayakaOno left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Duncan-Brain commented Mar 7, 2024 •

edited

Loading

kathyavini commented Mar 8, 2024 •

edited

Loading

kathyavini commented Mar 8, 2024 •

edited

Loading

kathyavini commented Mar 8, 2024 •

edited

Loading

kathyavini Mar 12, 2024 •

edited

Loading

Duncan-Brain Mar 13, 2024 •

edited

Loading