PLP filtering fixups (#63)

* Refactor SearchResultProvider state management This fixes a "flicker" of invalid product images being shown each time a filter was changed. A local variable was mutated to work around chaining `updateStore` calls carrying on the old store value. The reference of the `store` value in the action functions would carry the values of the initial store. Thus each time a color filter was changed for example then during the reloading phase the images for the initial products were shown. Use the functional component equivalent of a `setState` callback, which is an effect triggering on the relevant store change. Instead of functions chaining `updateStore` and calling `refresh()` trigger the refresh in an effect when the store has been updated to `reloading`. Also remove `appliedFilters`. The combination of `filters` and `reloading` provides the same functionality. * Update the total number of items when filtering * Bump version * Fix ForwardThumbnail throwing without img selector in children The main use case for this would be lazy loaded images.
storefront-foundation · Mar 20, 2020 · 6be4649 · 6be4649
1 parent cbfbd73
commit 6be4649
Show file tree

Hide file tree

Showing 7 changed files with 82 additions and 88 deletions.
diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "react-storefront",
-  "version": "7.9.1",
+  "version": "7.9.2",
   "description": "Build and deploy e-commerce progressive web apps (PWAs) in record time.",
   "module": "./index.js",
   "license": "Apache-2.0",

diff --git a/src/ForwardThumbnail.js b/src/ForwardThumbnail.js
@@ -25,11 +25,18 @@ export default function ForwardThumbnail({ children }) {
   const context = useContext(PWAContext)
   const srcRef = useRef(null)
 
-  useEffect(() => {
-    srcRef.current = ref.current.querySelector('img').getAttribute('src')
-  }, [children])
+  const setSrcRef = useCallback(() => {
+    if (ref.current.querySelector('img')) {
+      srcRef.current = ref.current.querySelector('img').getAttribute('src')
+    }
+  }, [])
+
+  useEffect(setSrcRef, [children])
 
   const handleClick = useCallback(() => {
+    if (!srcRef.current) {
+      setSrcRef()
+    }
     context.thumbnail.current = { src: srcRef.current }
   }, [])
 

diff --git a/src/plp/SearchResultsProvider.js b/src/plp/SearchResultsProvider.js
@@ -25,31 +25,43 @@ import replaceState from '../router/replaceState'
  */
 export default function SearchResultsProvider({ store, updateStore, children }) {
   useEffect(() => {
-    setState({
-      pageData: {
-        ...store.pageData,
-        appliedFilters: store.pageData.filters,
-      },
-    })
-  }, [])
-
-  const setState = state => {
-    store = { ...store, ...state }
-    updateStore(store)
-  }
+    if (store.reloading) {
+      async function refresh() {
+        const query = getQueryForState()
+        const apiUrl = getURLForState(query)
+
+        // Don't show page for user
+        delete query.page
+        replaceState(null, null, getURLForState(query))
+
+        const {
+          pageData: { products, total },
+        } = await fetch(`/api${apiUrl}`).then(res => res.json())
+        updateStore(store => ({
+          reloading: false,
+          pageData: {
+            ...store.pageData,
+            total,
+            products:
+              store.pageData.page === 0 ? products : store.pageData.products.concat(products),
+          },
+        }))
+      }
+      refresh()
+    }
+  }, [store])
 
   /**
    * Fetches the next page of results
    */
   const fetchMore = () => {
-    setState({
+    updateStore(store => ({
+      reloading: true,
       pageData: {
         ...store.pageData,
         page: store.pageData.page + 1,
       },
-    })
-
-    return refresh()
+    }))
   }
 
   /**
@@ -86,38 +98,33 @@ export default function SearchResultsProvider({ store, updateStore, children })
    * @param {Boolean} submit If true, fetches new results from the server
    */
   const setFilters = (filters, submit) => {
-    const { appliedFilters } = store.pageData
     const filtersChanged =
       JSON.stringify(filters.map(v => v.toLowerCase()).sort()) !==
-      JSON.stringify(appliedFilters.map(v => v.toLowerCase()).sort())
+      JSON.stringify(store.pageData.filters.map(v => v.toLowerCase()).sort())
 
-    setState({
+    updateStore(store => ({
+      reloading: Boolean(submit),
       pageData: {
         ...store.pageData,
         filters,
-        filtersChanged,
+        filtersChanged: submit ? false : filtersChanged,
+        page: submit ? 0 : store.pageData.page,
       },
-    })
-
-    if (submit) {
-      applyFilters()
-    }
+    }))
   }
 
   /**
    * Applies the selected filters, resets the page to 0 and fetches new results from the server.
    */
   const applyFilters = () => {
-    setState({
+    updateStore(store => ({
+      reloading: true,
       pageData: {
         ...store.pageData,
         filtersChanged: false,
-        appliedFilters: [...store.pageData.filters],
         page: 0,
       },
-    })
-
-    refresh()
+    }))
   }
 
   /**
@@ -162,45 +169,15 @@ export default function SearchResultsProvider({ store, updateStore, children })
     return pathname + qs.stringify(query, { addQueryPrefix: true }) + hash
   }
 
-  /**
-   * Fetches new results from the server
-   * @param {Object} options
-   */
-  const refresh = async () => {
-    const query = getQueryForState()
-    const apiUrl = getURLForState(query)
-
-    // Don't show page for user
-    delete query.page
-    replaceState(null, null, getURLForState(query))
-
-    if (store.pageData.page === 0) {
-      setState({ reloading: true })
-    }
-
-    const {
-      pageData: { products },
-    } = await fetch(`/api${apiUrl}`).then(res => res.json())
-
-    setState({
-      reloading: false,
-      pageData: {
-        ...store.pageData,
-        products: store.pageData.page === 0 ? products : store.pageData.products.concat(products),
-      },
-    })
-  }
-
   const setSort = option => {
-    setState({
+    updateStore(store => ({
+      reloading: true,
       pageData: {
         ...store.pageData,
         sort: option.code,
         page: 0,
       },
-    })
-
-    refresh()
+    }))
   }
 
   return (

diff --git a/src/plp/useSearchResultsStore.js b/src/plp/useSearchResultsStore.js
@@ -13,7 +13,6 @@ export default function useSearchResultsStore(lazyProps) {
       filters: [],
       sort: 'rating',
       sortSaved: 'rating',
-      appliedFilters: [],
       sortOptions: [],
       filtersChanged: false,
     }),

diff --git a/test/ForwardThumbnail.test.js b/test/ForwardThumbnail.test.js
@@ -30,6 +30,24 @@ describe('ForwardThumbnail', () => {
     expect(wrapper.find('#test2').prop('src')).toBe('test2')
   })
 
+  it('should render when no img selector in children', () => {
+    const Test = () => {
+      thumbnail = useRef(null)
+
+      return (
+        <PWAContext.Provider value={{ thumbnail }}>
+          <ForwardThumbnail>
+            <div id="test1">test1</div>
+          </ForwardThumbnail>
+        </PWAContext.Provider>
+      )
+    }
+
+    wrapper = mount(<Test />)
+
+    expect(wrapper.find('#test1').text()).toBe('test1')
+  })
+
   it('should set context thumbnail onClick', async () => {
     wrapper = mount(<Test />)
 

diff --git a/test/plp/SearchResultsProvider.test.js b/test/plp/SearchResultsProvider.test.js
@@ -9,9 +9,9 @@ describe('SearchResultsProvider', () => {
     pageData: {
       test: 'test',
       page: 0,
-      appliedFilters: [],
       filters: ['blue'],
       products: [{ id: 'first' }],
+      total: 50,
     },
   }
   let wrapper, context, getStore
@@ -81,7 +81,7 @@ describe('SearchResultsProvider', () => {
 
     it('toggleFilter - toggle existing filter', async () => {
       const facet = { code: 'blue' }
-      initialStore.pageData.appliedFilters = ['blue']
+      initialStore.pageData.filters = ['blue']
 
       wrapper = mount(<Test />)
 
@@ -92,8 +92,6 @@ describe('SearchResultsProvider', () => {
 
       expect(getStore.pageData.filters).toStrictEqual([])
       expect(getStore.pageData.filtersChanged).toBe(true)
-
-      initialStore.pageData.appliedFilters = []
     })
 
     it('toggleFilter - with submit', async () => {
@@ -113,9 +111,7 @@ describe('SearchResultsProvider', () => {
         await wrapper.update()
       })
 
-      expect(getStore.pageData.appliedFilters).toStrictEqual(
-        initialStore.pageData.filters.concat(facet.code),
-      )
+      expect(getStore.pageData.filters).toStrictEqual(['blue', 'red'])
       expect(getStore.pageData.filtersChanged).toBe(false)
       expect(fetch).toHaveBeenCalled()
     })
@@ -132,7 +128,6 @@ describe('SearchResultsProvider', () => {
     })
 
     it('clearFilters - with submit', async () => {
-      initialStore.pageData.appliedFilters = ['blue']
       fetchMock.mockResponseOnce(
         JSON.stringify({
           pageData: {
@@ -148,10 +143,7 @@ describe('SearchResultsProvider', () => {
       })
 
       expect(getStore.pageData.filters).toStrictEqual([])
-      expect(getStore.pageData.appliedFilters).toStrictEqual([])
       expect(fetch).toHaveBeenCalled()
-
-      initialStore.pageData.appliedFilters = []
     })
 
     it('refresh - should always remove "more" query param if sees it', async () => {
@@ -182,6 +174,7 @@ describe('SearchResultsProvider', () => {
         JSON.stringify({
           pageData: {
             products: [],
+            total: 0,
           },
         }),
       )
@@ -194,7 +187,7 @@ describe('SearchResultsProvider', () => {
 
       expect(getStore.pageData.filtersChanged).toBe(false)
       expect(getStore.pageData.page).toBe(0)
-      expect(getStore.pageData.appliedFilters).toStrictEqual(initialStore.pageData.filters)
+      expect(getStore.pageData.total).toBe(0)
       expect(fetch).toHaveBeenCalled()
     })