#525 List search: added SearchBar, added new filter to inventory #526

az1plo · 2024-11-25T10:43:41Z

What this PR does

Added SearchBar component for filtering lists.
Integrated SearchBar into the following sections:
- Inventory
- Shop
- Workshop
- Cows
- Cellar
Implemented a new filter logic for the inventory to enhance usability.

How this change can be validated

Navigate to each section listed above and use the search bar to filter items.
Verify that the displayed items match the search query.

Questions or concerns about this change

Are there sections where the SearchBar could be removed or added?

Additional information

…entory

vercel · 2024-11-25T10:43:45Z

@az1plo is attempting to deploy a commit to the Jeremy Kahn's projects Team on Vercel.

A member of the Team first needs to authorize it.

vercel · 2024-11-25T13:32:11Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
farmhand	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 29, 2024 6:00pm

jeremyckahn

Thank you for the amazing work so far @az1plo! This is working really well. There are a number of UI consistency and performance issues I'd like to see addressed before merging this. Hopefully my comments aren't too onerous to deal with. Feel free to ask any followup questions here or in Discord!

jeremyckahn · 2024-11-25T13:59:12Z

src/components/Cellar/CellarInventoryTabPanel.js

+    const itemName = item.name.toLowerCase()
+    const fermentationRecipeName = `${FERMENTED_CROP_NAME}${item.name}`.toLowerCase()


nit: Since this is a relatively hot code path, consider optimizing it a bit:

Suggested change

const itemName = item.name.toLowerCase()

const fermentationRecipeName = `${FERMENTED_CROP_NAME}${item.name}`.toLowerCase()

const fermentationRecipeName = `${FERMENTED_CROP_NAME}${itemName}`.toLowerCase()

jeremyckahn · 2024-11-25T14:06:05Z

src/components/Cellar/CellarInventoryTabPanel.js

  return (
    <TabPanel value={currentTab} index={index}>
      <h3>
-        Capacity: {integerString(cellarInventory.length)} /{' '}
+        Capacity: {integerString(filteredKegs.length)} /{' '}


It seems that this should be reverted to:

Suggested change

Capacity: {integerString(filteredKegs.length)} /{' '}

Capacity: {integerString(cellarInventory.length)} /{' '}

This text represents how much space the player has left to make recipes, and that won't change as they filter down the list.

jeremyckahn · 2024-11-25T14:15:40Z

src/components/CowPenContextMenu/CowPenContextMenu.js

@@ -105,98 +122,142 @@ export const CowPenContextMenu = ({
      </Tabs>
      <TabPanel value={currentTab} index={0}>
        <h3>
-          Capacity: {cowInventory.length} /{' '}
+          Capacity: {filteredCowInventory.length} /{' '}


Let's revert this as well to keep the information being displayed accurate:

Suggested change

Capacity: {filteredCowInventory.length} /{' '}

Capacity: {cowInventory.length} /{' '}

jeremyckahn · 2024-11-25T14:18:20Z

src/components/CowPenContextMenu/CowPenContextMenu.js

+
+          return (
+            <>
+              <h3>Capacity: {filteredCows.length} / 2</h3>


To keep the label accurate:

Suggested change

<h3>Capacity: {filteredCows.length} / 2</h3>

<h3>Capacity: {numberOfCowsBreeding(cowBreedingPen)} / 2</h3>

jeremyckahn · 2024-11-25T14:25:06Z

src/components/FermentationRecipeList/FermentationRecipeList.test.js

Thank you for updating the tests! 🙏

jeremyckahn · 2024-11-25T14:45:49Z

src/components/Inventory/Inventory.js

I'd like to see this updated to use the section order that is used prior to this PR. With this PR, it seems to be:

Upgrades

Seeds

Mined Resources

Foraged Items

Field Tools

Crops

Crafted Items

Animal Supplies

Animal Products

But I'd like to see this revert to:

Crops

Seeds

Foraged Items

Field Tools

Animal Products

Animal Supplies

Crafted Items

Mined Resources

Also, let's drop the new "Upgrades" section from here to maintain consistent behavior. This should keep the most frequently-accessed items towards the top of the page.

jeremyckahn · 2024-11-25T14:51:59Z

src/components/Inventory/Inventory.js

+                  <input
+                    type="checkbox"
+                    disabled={isPurchaseView}
+                    checked={selectedCategories.includes(key)}
+                    onChange={() => toggleCategory(key)}
                  />


In order to maintain consistency with the overall MUI theme, let's use MUI Checkboxes here:

https://mui.com/material-ui/react-checkbox/

jeremyckahn · 2024-11-25T14:53:01Z

src/components/Inventory/Inventory.js

+            className={`filter-container ${
+              filterVisible ? 'visible' : 'hidden'
+            }`}


Consider leveraging MUI Accordion instead of a custom implementation to maintain UI consistency:

https://mui.com/material-ui/react-accordion/

jeremyckahn · 2024-11-25T14:56:15Z

src/components/SearchBar/SearchBar.js

+    <div className="search-bar">
+      <input
+        type="text"
+        placeholder={placeholder || 'Search...'}
+        onChange={handleInputChange}
+      />
+    </div>


Let's use a MUI Text Field here to maintain UI consistency: https://mui.com/material-ui/react-text-field/

jeremyckahn · 2024-11-25T14:58:27Z

src/components/SearchBar/SearchBar.js

+  const handleInputChange = event => {
+    onSearch(event.target.value)
+  }


I noticed that searching can be a little sluggish, so let's debounce the input to keep the UI responsive. I'd recommend considering these options (and installing the packages as necessary):

https://usehooks-ts.com/react-hook/use-debounce-value

https://usehooks-ts.com/react-hook/use-debounce-callback

luke-createdbyhumans

[deleted]

lstebner

sorry i'm re-posting my comments from my personal account because i was logged in to the wrong account at first

i didn't have time to fully review yet, but one thing did stand out to me and i'm also seeing that when the search input is not in the left column (like when searching the kitchen or the shop) it needs some more horizontal padding to line up with the other items in that view.

it's great to see this PR though! i'll look at it more closely when i can

lstebner · 2024-11-25T17:40:11Z

src/components/FermentationRecipeList/FermentationRecipeList.test.js

+    )
+    expect(searchBar).toBeInTheDocument()
+
+    fireEvent.change(searchBar, { target: { value: 'apple' } })


if possible, we should use userEvent.type instead of fireEvent. note: it requires an await. docs here https://github.com/testing-library/user-event (i'm only going to leave this comment here, but i see fireEvent in some other tests that i think could be updated as well)

jeremyckahn

Great work @az1plo! There's just a few more things to tighten up before this can be merged. I think we're almost there!

jeremyckahn · 2024-11-26T14:28:24Z

src/components/SearchBar/SearchBar.js

@@ -1,18 +1,28 @@
 import React from 'react'
 import PropTypes from 'prop-types'
+import { useDebounceCallback } from 'usehooks-ts'
+import TextField from '@mui/material/TextField.js'


This import appears to be broken: https://vercel.com/jeremy-kahns-projects-98e6f140/farmhand/Fd1vDBBPZuNxA1FywnBNj9cqnp4a?utm_medium=email&utm_source=notifications&filter=errors#L141

I think this needs to be:

Suggested change

import TextField from '@mui/material/TextField.js'

import TextField from '@mui/material/TextField/index.js'

src/components/Inventory/Inventory.js

jeremyckahn · 2024-11-26T14:48:33Z

src/components/Inventory/Inventory.js

+export const categoryIds = {
+  CROPS: 'CROPS',
+  SEEDS: 'SEEDS',
+  FORAGED_ITEMS: 'FORAGED_ITEMS',
+  FIELD_TOOLS: 'FIELD_TOOLS',
+  ANIMAL_PRODUCTS: 'ANIMAL_PRODUCTS',
+  ANIMAL_SUPPLIES: 'ANIMAL_SUPPLIES',
+  CRAFTED_ITEMS: 'CRAFTED_ITEMS',
+  MINED_RESOURCES: 'MINED_RESOURCES',
+}


Since we are depending on the insertion order of these keys, let's use a Map instead of a plain object here. It's likely that this implementation would work as expected, but it's not necessarily obvious that the key order matters (because it usually doesn't for a plain Object). Using a Map would more clearly indicate to future readers of this code that the key order is significant (and perhaps a comment describing as much to really drive the point home).

jeremyckahn · 2024-11-26T14:52:55Z

src/components/Inventory/Inventory.test.js

-      renderedItems = component.find(Item)
-      expect(renderedItems).toHaveLength(1)
-      expect(renderedItems.props().item.id).toBe(carrot.id)
+      // Проверяем заполнение всех категорий


Could this comment be translated to English for consistency?

jeremyckahn · 2024-11-26T14:53:14Z

src/components/Inventory/Inventory.test.js

+        [categoryIds.ANIMAL_PRODUCTS]: [testItem({ id: 'milk-1' })],
+        [categoryIds.ANIMAL_SUPPLIES]: [testItem({ id: 'cow-feed' })],
+        [categoryIds.CRAFTED_ITEMS]: [testItem({ id: carrotSoup.id })],
+        [categoryIds.UPGRADES]: [], // Пустая категория


This comment as well, please. 🙏

jeremyckahn · 2024-11-26T14:54:34Z

src/components/SearchBar/SearchBar.js

+  const debouncedSearch = useDebounceCallback(value => {
+    onSearch(value)
+  }, 300)


Thank you for making this change!

jeremyckahn · 2024-11-26T14:57:55Z

src/components/Inventory/Inventory.js

+                <FormControlLabel
+                  key={key}
+                  control={


Let's adjust this to keep each option on its own line:

Suggested change

<FormControlLabel

key={key}

control={

<FormControlLabel

sx={{ display: 'block' }}

key={key}

control={

Before:

After:

jeremyckahn · 2024-11-26T14:58:21Z

src/components/SearchBar/SearchBar.js

        placeholder={placeholder || 'Search...'}
        onChange={handleInputChange}
+        inputProps={{
+          'aria-label': 'search',


jeremyckahn · 2024-11-26T15:00:25Z

src/components/SearchBar/SearchBar.test.js


-    expect(onSearchMock).toHaveBeenCalledTimes(1)
-    expect(onSearchMock).toHaveBeenCalledWith('test query')
+    // Проверяем debounce


Let's translate this comment to English as well. 🙂

jeremyckahn

Thanks for the latest changes @az1plo! There's a few small test-related issues to iron out, but the application code is looking good. 🙂

jeremyckahn · 2024-11-27T14:06:30Z

package.json

@@ -73,6 +73,7 @@
    "eslint-plugin-react-hooks": "^2.5.0",
    "git-branch-is": "^4.0.0",
    "husky": "^4.2.3",
+    "jest": "^29.7.0",


Let remove the inclusion of Jest here. This project uses Vitest, so this package isn't necessary.

It looks like this still needs to be reverted out of this PR.

jeremyckahn · 2024-11-27T14:06:46Z

src/components/Inventory/Inventory.js

-  CRAFTED_ITEMS: 'CRAFTED_ITEMS',
-  MINED_RESOURCES: 'MINED_RESOURCES',
-}
+// Using Map to preserve the key order


jeremyckahn · 2024-11-27T14:08:34Z

src/components/Inventory/Inventory.test.js

+      const { container } = render(
+        <Inventory items={items} selectedCategories={[]} />
+      )
+      console.log(container.innerHTML)


Let's remove this to keep test output clean.

jeremyckahn · 2024-11-27T14:23:06Z

src/components/Inventory/Inventory.test.js

It looks like there are two test failures happening in this file. These will need to be resolved before this can be merged.

This would have been caught by the CI system, but it appears that the tests aren't being run for this PR. It's not clear why that is, as all the configuration and GitHub Actions setup seems correct. Since GitHub doesn't appear to be cooperating, you'll have to run the tests manually, locally. 😕

jeremyckahn

I pushed fdef440 to address the test failures, so we should be good there.

Just a few more test-related items to address and this will be good to merge!

jeremyckahn · 2024-11-28T20:45:08Z

src/components/Inventory/Inventory.test.js


-  test('divides into type categories', () => {


This test seems valuable. Would you mind reverting this removal (and make any necessary updates to make it pass)?

jeremyckahn · 2024-11-28T20:49:58Z

package.json

@@ -73,6 +73,7 @@
    "eslint-plugin-react-hooks": "^2.5.0",
    "git-branch-is": "^4.0.0",
    "husky": "^4.2.3",
+    "jest": "^29.7.0",


It looks like this still needs to be reverted out of this PR.

… list-search

jeremyckahn

Great work @az1plo! This looks and works great. Approved! 👏

I'm going to merge this now, but I'm going to hold off on releasing it to Production because I'd like to get #527 merged ahead of the next release. Releasing the changes together will result in less update churn for players. I should have a fix in for that soon and will announce the release on Discord.

Thanks again for the amazing contribution! 🙌

jeremyckahn · 2024-11-29T17:43:10Z

src/components/Inventory/Inventory.test.js

-    expect(
-      sortItems([
+  describe('Categorizing items', () => {
+    test('divides items into type categories', () => {


Awesome! Thanks for adding this. 🙂

* chore(tsconfig): set moduleResolution: node * fix(#527): enable cow renaming * fix(#526): improve flakey inventory test * refactor(#527): use usehooks-ts implementation of isMounted

jeremyckahn#525 List search: added SearchBar, added new filter to inv…

4e99d8d

…entory

vercel bot deployed to Preview November 25, 2024 13:33 View deployment

jeremyckahn requested changes Nov 25, 2024

View reviewed changes

luke-createdbyhumans reviewed Nov 25, 2024

View reviewed changes

lstebner reviewed Nov 25, 2024

View reviewed changes

jeremyckahn#525 List search: requested changes

d0c3a09

vercel bot had a problem deploying to Preview November 26, 2024 14:24 Failure

jeremyckahn requested changes Nov 26, 2024

View reviewed changes

az1plo added 2 commits November 27, 2024 05:03

List search: requested changes

9a00615

List search: requested changes

6419076

jeremyckahn requested changes Nov 27, 2024

View reviewed changes

fix(inventory): make tests pass

fdef440

vercel bot deployed to Preview November 28, 2024 16:05 View deployment

jeremyckahn requested changes Nov 28, 2024

View reviewed changes

az1plo and others added 5 commits November 29, 2024 01:19

removed jest

f737fd5

Merge branch 'list-search' of https://github.com/az1plo/farmhand into…

2b59b7f

… list-search

Fixed seeds and crops categories, added test as requested.

8f23e77

Fixed seeds and crops categories, added test as requested.

0fea419

refactor(inventory): use module import paths

e9f2ed8

jeremyckahn approved these changes Nov 29, 2024

View reviewed changes

vercel bot deployed to Preview November 29, 2024 18:00 View deployment

jeremyckahn merged commit 0e0be98 into jeremyckahn:develop Nov 29, 2024
3 checks passed

jeremyckahn added a commit that referenced this pull request Nov 29, 2024

fix(#526): improve flakey inventory test

56f72dc

jeremyckahn mentioned this pull request Nov 29, 2024

fix(closes #527): Enable cow renaming #528

Merged

jeremyckahn added a commit that referenced this pull request Dec 1, 2024

fix(#526): improve flakey inventory test

b8fe71e

		const itemName = item.name.toLowerCase()
		const fermentationRecipeName = `${FERMENTED_CROP_NAME}${item.name}`.toLowerCase()

	Capacity: {integerString(filteredKegs.length)} /{' '}
	Capacity: {integerString(cellarInventory.length)} /{' '}

	Capacity: {filteredCowInventory.length} /{' '}
	Capacity: {cowInventory.length} /{' '}

	<h3>Capacity: {filteredCows.length} / 2</h3>
	<h3>Capacity: {numberOfCowsBreeding(cowBreedingPen)} / 2</h3>

	import TextField from '@mui/material/TextField.js'
	import TextField from '@mui/material/TextField/index.js'

#525 List search: added SearchBar, added new filter to inventory #526

#525 List search: added SearchBar, added new filter to inventory #526

Conversation

az1plo commented Nov 25, 2024

Questions or concerns about this change

Additional information

vercel bot commented Nov 25, 2024

vercel bot commented Nov 25, 2024 • edited Loading

jeremyckahn 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

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

luke-createdbyhumans left a comment • edited Loading

Choose a reason for hiding this comment

lstebner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremyckahn 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

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

jeremyckahn 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremyckahn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremyckahn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented Nov 25, 2024 •

edited

Loading

luke-createdbyhumans left a comment •

edited

Loading