From 5db8a50f3bda6287f35aa0f88bf2ac3fba86426c Mon Sep 17 00:00:00 2001 From: Tommy Wiebell Date: Thu, 30 May 2019 10:09:09 -0500 Subject: [PATCH] Category RootComponent Simple Re-Factor (#1211) * Commit copy/pasta attempt of re-factor that is not working as expected * Move classify hook to function body, le sigh * Comment out scrollTo * Finish re-factor to functional components and update tests * - Move pagination logic to custom hook - Refactor Category component to use this hook and simplify render logic * Address some additional feedback * Add some state checking before bounds validation (should be temporary until pagination refactor) * Remove obsolete test that is failing * Make sure child component doesn't render until total pages is set * Remove comment :upside_down_face: * Add comment explaining loading indicator logic --- packages/peregrine/src/hooks/usePagination.js | 14 ++ packages/peregrine/src/index.js | 1 + .../categoryContent.spec.js.snap | 40 ++++ .../__tests__/categoryContent.spec.js | 15 +- .../src/RootComponents/Category/category.js | 171 +++++++----------- .../Category/categoryContent.js | 59 +++--- .../Pagination/__tests__/pagination.spec.js | 10 - .../src/components/Pagination/pagination.js | 5 - .../src/queries/getCategory.graphql | 3 + 9 files changed, 163 insertions(+), 155 deletions(-) create mode 100644 packages/peregrine/src/hooks/usePagination.js create mode 100644 packages/venia-concept/src/RootComponents/Category/__tests__/__snapshots__/categoryContent.spec.js.snap diff --git a/packages/peregrine/src/hooks/usePagination.js b/packages/peregrine/src/hooks/usePagination.js new file mode 100644 index 0000000000..20932c1cda --- /dev/null +++ b/packages/peregrine/src/hooks/usePagination.js @@ -0,0 +1,14 @@ +import { useMemo, useState } from 'react'; + +export const usePagination = () => { + const [currentPage, setCurrentPage] = useState(0); + const [totalPages, setTotalPages] = useState(null); + + const paginationState = { currentPage, totalPages }; + const api = useMemo(() => ({ setCurrentPage, setTotalPages }), [ + setCurrentPage, + setTotalPages + ]); + + return [paginationState, api]; +}; diff --git a/packages/peregrine/src/index.js b/packages/peregrine/src/index.js index d5d3d1738c..2011ab0b5a 100644 --- a/packages/peregrine/src/index.js +++ b/packages/peregrine/src/index.js @@ -5,6 +5,7 @@ import * as Util from './util'; export { useApolloContext } from './hooks/useApolloContext'; export { useEventListener } from './hooks/useEventListener'; export { useDropdown } from './hooks/useDropdown'; +export { usePagination } from './hooks/usePagination'; export { useQuery } from './hooks/useQuery'; export { useQueryResult } from './hooks/useQueryResult'; export { useSearchParam } from './hooks/useSearchParam'; diff --git a/packages/venia-concept/src/RootComponents/Category/__tests__/__snapshots__/categoryContent.spec.js.snap b/packages/venia-concept/src/RootComponents/Category/__tests__/__snapshots__/categoryContent.spec.js.snap new file mode 100644 index 0000000000..f751c3e0dc --- /dev/null +++ b/packages/venia-concept/src/RootComponents/Category/__tests__/__snapshots__/categoryContent.spec.js.snap @@ -0,0 +1,40 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`renders the correct tree 1`] = ` +
+

+
+
+

+
+ +
+
+ +
+
+`; diff --git a/packages/venia-concept/src/RootComponents/Category/__tests__/categoryContent.spec.js b/packages/venia-concept/src/RootComponents/Category/__tests__/categoryContent.spec.js index 174d5bb88c..69ab9cd6c6 100644 --- a/packages/venia-concept/src/RootComponents/Category/__tests__/categoryContent.spec.js +++ b/packages/venia-concept/src/RootComponents/Category/__tests__/categoryContent.spec.js @@ -1,5 +1,5 @@ import React from 'react'; -import { shallow } from 'enzyme'; +import ShallowRenderer from 'react-test-renderer/shallow'; import CategoryContent from '../categoryContent'; @@ -21,15 +21,16 @@ const data = { } }; -test('renders with props', () => { - const pageSize = 6; - const wrapper = shallow( +test('renders the correct tree', () => { + const shallowRenderer = new ShallowRenderer(); + const tree = shallowRenderer.render( - ).dive(); - expect(wrapper.hasClass(classes.root)).toBe(true); + ); + + expect(tree).toMatchSnapshot(); }); diff --git a/packages/venia-concept/src/RootComponents/Category/category.js b/packages/venia-concept/src/RootComponents/Category/category.js index 94fe48ac2e..173c4fca58 100644 --- a/packages/venia-concept/src/RootComponents/Category/category.js +++ b/packages/venia-concept/src/RootComponents/Category/category.js @@ -1,119 +1,84 @@ -import React, { Component } from 'react'; +import React, { useEffect } from 'react'; import { string, number, shape } from 'prop-types'; -import { compose } from 'redux'; -import { connect, Query } from 'src/drivers'; +import { usePagination, useQuery } from '@magento/peregrine'; -import classify from 'src/classify'; -import { setCurrentPage, setPrevPageTotal } from 'src/actions/catalog'; -import { loadingIndicator } from 'src/components/LoadingIndicator'; +import { mergeClasses } from 'src/classify'; +import categoryQuery from 'src/queries/getCategory.graphql'; import CategoryContent from './categoryContent'; +import { loadingIndicator } from 'src/components/LoadingIndicator'; import defaultClasses from './category.css'; -import categoryQuery from 'src/queries/getCategory.graphql'; -class Category extends Component { - static propTypes = { - id: number, - classes: shape({ - gallery: string, - root: string, - title: string - }), - currentPage: number, - pageSize: number, - prevPageTotal: number - }; +const Category = props => { + const { id, pageSize } = props; - // TODO: Should not be a default here, we just don't have - // the wiring in place to map route info down the tree (yet) - static defaultProps = { - id: 3 + const [paginationValues, paginationApi] = usePagination(); + const { currentPage, totalPages } = paginationValues; + const { setCurrentPage, setTotalPages } = paginationApi; + + const pageControl = { + currentPage, + setPage: setCurrentPage, + updateTotalPages: setTotalPages, + totalPages }; - componentDidUpdate(prevProps) { - // If the current page has changed, scroll back up to the top. - if (this.props.currentPage !== prevProps.currentPage) { - window.scrollTo(0, 0); - } - } + const [queryResult, queryApi] = useQuery(categoryQuery); + const { data, error, loading } = queryResult; + const { runQuery, setLoading } = queryApi; + const classes = mergeClasses(defaultClasses, props.classes); - render() { - const { - id, - classes, - currentPage, - pageSize, - prevPageTotal, - setCurrentPage, - setPrevPageTotal - } = this.props; + useEffect(() => { + setLoading(true); + runQuery({ + variables: { + id: Number(id), + onServer: false, + pageSize: Number(pageSize), + currentPage: Number(currentPage) + } + }); - const pageControl = { - currentPage: currentPage, - setPage: setCurrentPage, - updateTotalPages: setPrevPageTotal, - totalPages: prevPageTotal - }; + window.scrollTo({ + left: 0, + top: 0, + behavior: 'smooth' + }); + }, [id, pageSize, currentPage]); - return ( - - {({ loading, error, data }) => { - if (error) return
Data Fetch Error
; - // If our pagination component has mounted, then we have - // a total page count in the store, so we continue to render - // with our last known total - if (loading) - return pageControl.totalPages ? ( - - ) : ( - loadingIndicator - ); + const totalPagesFromData = data + ? data.category.products.page_info.total_pages + : null; + useEffect(() => { + setTotalPages(totalPagesFromData); + }, [totalPagesFromData]); - // TODO: Retrieve the page total from GraphQL when ready - const pageCount = - data.category.products.total_count / pageSize; - const totalPages = Math.ceil(pageCount); - const totalWrapper = { - ...pageControl, - totalPages: totalPages - }; + if (error) return
Data Fetch Error
; + // show loading indicator until our data has been fetched and pagination state has been updated + if (!totalPages) return loadingIndicator; - return ( - - ); - }} - - ); - } -} + // if our data is still loading, we want to reset our data state to null + return ( + + ); +}; -const mapStateToProps = ({ catalog }) => { - return { - currentPage: catalog.currentPage, - pageSize: catalog.pageSize, - prevPageTotal: catalog.prevPageTotal - }; +Category.propTypes = { + id: number, + classes: shape({ + gallery: string, + root: string, + title: string + }), + pageSize: number +}; + +Category.defaultProps = { + id: 3, + pageSize: 6 }; -const mapDispatchToProps = { setCurrentPage, setPrevPageTotal }; -export default compose( - classify(defaultClasses), - connect( - mapStateToProps, - mapDispatchToProps - ) -)(Category); +export default Category; diff --git a/packages/venia-concept/src/RootComponents/Category/categoryContent.js b/packages/venia-concept/src/RootComponents/Category/categoryContent.js index 8f16bf20e4..148d2544a0 100644 --- a/packages/venia-concept/src/RootComponents/Category/categoryContent.js +++ b/packages/venia-concept/src/RootComponents/Category/categoryContent.js @@ -1,36 +1,35 @@ -import React, { Component } from 'react'; -import classify from 'src/classify'; +import React from 'react'; +import { mergeClasses } from 'src/classify'; import Gallery from 'src/components/Gallery'; import Pagination from 'src/components/Pagination'; import defaultClasses from './category.css'; -class CategoryContent extends Component { - render() { - const { classes, pageControl, data, pageSize } = this.props; - const items = data ? data.category.products.items : null; - const title = data ? data.category.description : null; - const categoryTitle = data ? data.category.name : null; +const CategoryContent = props => { + const { pageControl, data, pageSize } = props; + const classes = mergeClasses(defaultClasses, props.classes); + const items = data ? data.category.products.items : null; + const title = data ? data.category.description : null; + const categoryTitle = data ? data.category.name : null; - return ( -
-

- {/* TODO: Switch to RichContent component from Peregrine when merged */} -
-
{categoryTitle}
-

-
- -
-
- -
-
- ); - } -} + return ( +
+

+ {/* TODO: Switch to RichContent component from Peregrine when merged */} +
+
{categoryTitle}
+

+
+ +
+
+ +
+
+ ); +}; -export default classify(defaultClasses)(CategoryContent); +export default CategoryContent; diff --git a/packages/venia-concept/src/components/Pagination/__tests__/pagination.spec.js b/packages/venia-concept/src/components/Pagination/__tests__/pagination.spec.js index 67710b9d8c..97af945b0b 100644 --- a/packages/venia-concept/src/components/Pagination/__tests__/pagination.spec.js +++ b/packages/venia-concept/src/components/Pagination/__tests__/pagination.spec.js @@ -185,13 +185,3 @@ test('rightSkip skips toward last page', () => { expect(setPage).toHaveBeenCalledTimes(1); expect(setPage).toHaveBeenLastCalledWith(8); }); - -test('updateTotalPages is called on component mount', () => { - TestRenderer.create( - - - - ); - - expect(updateTotalPages).toHaveBeenCalledTimes(1); -}); diff --git a/packages/venia-concept/src/components/Pagination/pagination.js b/packages/venia-concept/src/components/Pagination/pagination.js index 3206e63b87..6d256b22c8 100644 --- a/packages/venia-concept/src/components/Pagination/pagination.js +++ b/packages/venia-concept/src/components/Pagination/pagination.js @@ -27,11 +27,6 @@ class Pagination extends Component { }; componentDidMount() { - // updateTotalPages pushes the current page count of a category query to - // redux so it knows how many tiles to render even in the Query - // component's loading state - const { updateTotalPages, totalPages } = this.props.pageControl; - updateTotalPages(totalPages); this.syncPage(); } diff --git a/packages/venia-concept/src/queries/getCategory.graphql b/packages/venia-concept/src/queries/getCategory.graphql index 97762d5248..22b56ce9d9 100644 --- a/packages/venia-concept/src/queries/getCategory.graphql +++ b/packages/venia-concept/src/queries/getCategory.graphql @@ -21,6 +21,9 @@ query category($id: Int!, $pageSize: Int!, $currentPage: Int!, $onServer: Boolea } } } + page_info { + total_pages + } total_count } meta_title @include(if: $onServer)