Skip to content
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

Lf 4083 add search to animal list #3152

Merged
merged 23 commits into from
Mar 13, 2024
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
dcd2a5c
LF-4083 - Add opt-out for collapse behaviour of search
Duncan-Brain Mar 6, 2024
d228053
LF-4083 - Add search to Animal Inventory , fix useSearchFilter jsDoc
Duncan-Brain Mar 6, 2024
99e8508
Merge branch 'LF-4103-implement-animals-table' into LF-4083-add-searc…
Duncan-Brain Mar 6, 2024
aeca840
Merge branch 'integration' into LF-4083-add-search-to-animal-list
Duncan-Brain Mar 7, 2024
bb11c74
LF-4083 - Fix a few styling issues
Duncan-Brain Mar 7, 2024
ab76695
LF-4083 - add translations, move search hooks to container
Duncan-Brain Mar 11, 2024
311b2a3
LF-4083 - make new searchbar type
Duncan-Brain Mar 11, 2024
3be636f
LF-4083 - remove excess styles
Duncan-Brain Mar 11, 2024
f0b16ca
LF-4083 - fix strings and copyright
Duncan-Brain Mar 12, 2024
1fbe4a6
LF-4083 - fix weird tablet behaviour, conditional zIndexing
Duncan-Brain Mar 12, 2024
68564b7
LF-4083 - Remove collapse from old component, and update stories
Duncan-Brain Mar 12, 2024
dd75c2a
LF-4083 rename search filter
Duncan-Brain Mar 12, 2024
af075d7
LF-4083 - update naming
Duncan-Brain Mar 12, 2024
10ac382
LF-4083 - small removals and fixes
Duncan-Brain Mar 12, 2024
f783e27
LF-4083 - remove unused imports, correct conditionals
Duncan-Brain Mar 12, 2024
4a79dbf
LF-4083 - remove unused style
Duncan-Brain Mar 12, 2024
24af1bd
LF-4083 - use correct i18n syntax and copyright
Duncan-Brain Mar 12, 2024
403bfdc
LF-4083 - update styles naming, condense code
Duncan-Brain Mar 12, 2024
4891cd4
LF-4083 - remove ununsed type
Duncan-Brain Mar 12, 2024
4273ef4
Merge branch 'integration' into LF-4083-add-search-to-animal-list
Duncan-Brain Mar 13, 2024
3d399ca
LF-4083 - fix copyright
Duncan-Brain Mar 13, 2024
8c781fb
LF-4083 - remove inconsistent styles
Duncan-Brain Mar 13, 2024
62bb1a7
LF-4083 - missed merge conflict repurcussion
Duncan-Brain Mar 13, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion packages/webapp/public/locales/en/translation.json
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,10 @@
"INDIVIDUALS": "Individuals",
"MALE": "Male",
"TITLE": "Filter animals"
}
},
"SEARCH_INVENTORY_PLACEHOLDER": "Search your inventory",
"SHOWING_RESULTS_WITH_COUNT_one": "Showing {{count}} result",
"SHOWING_RESULTS_WITH_COUNT_other": "Showing {{count}} results"
},
"BED_PLAN": {
"LENGTH_OF_BED": "Length of beds",
Expand Down
5 changes: 4 additions & 1 deletion packages/webapp/public/locales/es/translation.json
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,10 @@
"INDIVIDUALS": "MISSING",
"MALE": "MISSING",
"TITLE": "MISSING"
}
},
"SEARCH_INVENTORY_PLACEHOLDER": "MISSING",
"SHOWING_RESULTS_WITH_COUNT_one": "MISSING",
"SHOWING_RESULTS_WITH_COUNT_other": "MISSING"
},
"BED_PLAN": {
"LENGTH_OF_BED": "Largo de camas",
Expand Down
5 changes: 4 additions & 1 deletion packages/webapp/public/locales/fr/translation.json
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,10 @@
"INDIVIDUALS": "MISSING",
"MALE": "MISSING",
"TITLE": "MISSING"
}
},
"SEARCH_INVENTORY_PLACEHOLDER": "MISSING",
"SHOWING_RESULTS_WITH_COUNT_one": "MISSING",
"SHOWING_RESULTS_WITH_COUNT_other": "MISSING"
},
"BED_PLAN": {
"LENGTH_OF_BED": "Longueur des plates-bandes",
Expand Down
5 changes: 4 additions & 1 deletion packages/webapp/public/locales/pt/translation.json
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,10 @@
"INDIVIDUALS": "MISSING",
"MALE": "MISSING",
"TITLE": "MISSING"
}
},
"SEARCH_INVENTORY_PLACEHOLDER": "MISSING",
"SHOWING_RESULTS_WITH_COUNT_one": "MISSING",
"SHOWING_RESULTS_WITH_COUNT_other": "MISSING"
},
"BED_PLAN": {
"LENGTH_OF_BED": "Comprimento dos canteiros ",
Expand Down
93 changes: 72 additions & 21 deletions packages/webapp/src/components/Animals/Inventory/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,44 +14,95 @@
*/
import Table from '../../../components/Table';
import Layout from '../../../components/Layout';
import PureSearchBarWithBackdrop from '../../PopupFilter/PureSearchWithBackdrop';
import NoSearchResults from '../../../components/Card/NoSearchResults';
import type { AnimalInventory } from '../../../containers/Animals/Inventory/useAnimalInventory';
import { TableV2Column, TableKind } from '../../Table/types';
import type { DefaultTheme } from '@mui/styles';
import type { Dispatch, SetStateAction } from 'react';
import styles from './styles.module.scss';
import clsx from 'clsx';

export type SearchProps = {
searchString: string | null | undefined;
setSearchString: Dispatch<SetStateAction<string[]>>;
placeHolderText: string;
searchResultsText: string;
};

const PureAnimalInventory = ({
tableData,
filteredInventory,
animalsColumns,
theme,
isMobile,
zIndexBase,
backgroundColor,
isDesktop,
searchProps,
}: {
tableData: AnimalInventory[];
filteredInventory: AnimalInventory[];
animalsColumns: TableV2Column[];
theme: DefaultTheme;
isMobile: boolean;
zIndexBase: number;
backgroundColor: string;
isDesktop: boolean;
searchProps: SearchProps;
}) => {
const { searchString, setSearchString, placeHolderText, searchResultsText } = searchProps;
const hasSearchResults = filteredInventory.length !== 0;

return (
<Layout
classes={{
container: {
backgroundColor: theme.palette.background.paper,
borderRadius: '8px',
border: '1px solid var(--Colors-Primary-Primary-teal-50)',
marginTop: '16px',
backgroundColor: backgroundColor,
borderRadius: isDesktop && '8px',
border: isDesktop && '1px solid var(--Colors-Primary-Primary-teal-50)',
marginTop: isDesktop && '16px',
padding: !isDesktop ? '0px' : '16px',
},
}}
hasWhiteBackground
footer={false}
>
<Table
kind={TableKind.V2}
alternatingRowColor={true}
columns={animalsColumns}
data={tableData}
shouldFixTableLayout={!isMobile}
minRows={tableData.length}
dense={false}
showHeader={!isMobile}
/>
<div
className={clsx(
isDesktop ? styles.SearchAndFilter_container___desktop : styles.SearchAndFilter_container,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

)}
>
<PureSearchBarWithBackdrop
value={searchString}
onChange={(e: any) => setSearchString(e.target.value)}
isSearchActive={!!searchString}
placeholderText={placeHolderText}
zIndexBase={zIndexBase}
isDesktop={isDesktop}
className={clsx(isDesktop ? styles.SearchBar___widthFixed : styles.SearchBar___widthFull)}
/>
<div
className={clsx(
isDesktop ? styles.SearchResults___marginLeft8 : styles.SearchResults___textAlignCenter,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@Duncan-Brain Duncan-Brain Mar 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

)}
>
{searchResultsText}
</div>
</div>
{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.NoSearchResults___marginTop40 : styles.NoSearchResults__marginTop24,
)}
searchTerm={searchString}
includeFiltersInClearSuggestion
/>
)}
</Layout>
);
};
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
/*
* Copyright 2024 LiteFarm.org
* This file is part of LiteFarm.
*
* LiteFarm is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* LiteFarm is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details, see <https://www.gnu.org/licenses/>.
*/

.NoSearchResults___marginTop24 {
margin-top: 24px;
}

.NoSearchResults___marginTop40 {
margin-top: 40px;
}

.SearchBar___widthFixed {
width: 240px;
}

.SearchBar___widthFull {
width: 100%;
}

.SearchAndFilter_container___desktop {
display: inline-flex;
align-items: center;
gap: 8px;
margin-bottom: 16px;
}

.SearchAndFilter_container {
display: inline-grid;
gap: 4px;
padding: 8px 16px 8px 16px;
background-color: var(--Colors-Primary-Primary-teal-50);
}

.SearchResults___marginLeft8 {
margin-left: 8px;
}

.SearchResults___textAlignCenter {
text-align: center;
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2023 LiteFarm.org
* Copyright 2023-2024 LiteFarm.org
* This file is part of LiteFarm.
*
* LiteFarm is free software: you can redistribute it and/or modify
Expand All @@ -22,13 +22,14 @@ import Input from '../../Form/Input';
import TextButton from '../../Form/Button/TextButton';
import { Modal } from '../../Modals';

export default function PureCollapsibleSearch({
export default function PureCollapsingSearch({
value,
isSearchActive,
onChange,
placeholderText,
className,
containerRef,
isDesktop,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't use the breakpoint?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) Screenshot 2024-03-12 at 4 30 27 PM
    • 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)
      Screenshot 2024-03-12 at 4 31 20 PM

Copy link
Collaborator

@kathyavini kathyavini Mar 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}) {
const searchRef = useRef(null);
const [modalStyle, setModalStyle] = useState({});
Expand Down Expand Up @@ -82,7 +83,10 @@ export default function PureCollapsibleSearch({
}, [searchOverlayOpen]);

return (
<div className={clsx(styles.container, className)} ref={searchRef}>
<div
className={clsx(styles.container, className, isDesktop && styles.desktopContainer)}
ref={searchRef}
>
<Input
className={styles.searchBar}
isSearchBar
Expand All @@ -95,7 +99,7 @@ export default function PureCollapsibleSearch({
<Modal dismissModal={onSearchClose} style={modalStyle}>
<form // allows closing modal with enter key (= submit)
onSubmit={handleSubmit}
className={styles.modalContent}
className={clsx(styles.modalContent, isDesktop && styles.displayNone)}
>
<Input
className={styles.modalSearchbar}
Expand All @@ -109,32 +113,37 @@ export default function PureCollapsibleSearch({
)}

<TextButton
className={clsx(styles.searchButton, isSearchActive && styles.active)}
className={clsx(
styles.searchButton,
isSearchActive && styles.active,
isDesktop && styles.displayNone,
)}
onClick={onSearchOpen}
>
<SearchIcon className={styles.searchIcon} />
</TextButton>

{isSearchActive && (
<div className={styles.circleContainer}>
<div className={styles.circle} />
<div className={clsx(styles.circleContainer, isDesktop && styles.displayNone)}>
<div className={clsx(styles.circle, isDesktop && styles.displayNone)} />
</div>
)}
</div>
);
}

PureCollapsibleSearch.propTypes = {
PureCollapsingSearch.propTypes = {
value: PropTypes.string,
isSearchActive: PropTypes.bool,
onChange: PropTypes.func,
placeholderText: PropTypes.string,
className: PropTypes.string,
overlayModalOnButton: PropTypes.bool,
containerRef: PropTypes.shape({ current: PropTypes.instanceOf(Element) }),
isDesktop: PropTypes.bool,
};

PureCollapsibleSearch.defaultProps = {
PureCollapsingSearch.defaultProps = {
placeholderText: '',
isSearchActive: false,
className: '',
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2023 LiteFarm.org
* Copyright 2023-2024 LiteFarm.org
* This file is part of LiteFarm.
*
* LiteFarm is free software: you can redistribute it and/or modify
Expand Down Expand Up @@ -96,24 +96,15 @@
}
}

/* Desktop view */
@media only screen and (min-width: 768px) {
.container {
.searchBar {
display: block;
min-width: 240px;
flex-grow: 1;
}
}

.searchButton,
.circleContainer,
.circle {
display: none;
.desktopContainer {
.searchBar {
display: block;
min-width: 240px;
flex-grow: 1;
}
}

// Only relevant if resizing screen beyond breakpoint with modal open
.modalContent {
display: none;
}
.displayNone {
display: none;
}

Loading
Loading