-
Notifications
You must be signed in to change notification settings - Fork 683
Category RootComponent Simple Re-Factor #1211
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
Merged
Merged
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
9b4c384
Commit copy/pasta attempt of re-factor that is not working as expected
tjwiebell d8196a4
Move classify hook to function body, le sigh
tjwiebell e22449d
Comment out scrollTo
tjwiebell 476f3f1
Finish re-factor to functional components and update tests
tjwiebell 58bf86c
Merge remote-tracking branch 'mainline/develop' into feature/category…
tjwiebell d54790e
- Move pagination logic to custom hook
tjwiebell 96f5475
Address some additional feedback
tjwiebell 8b13a9f
Merge branch 'develop' into feature/category-peregrine
devpatil7 7305021
Add some state checking before bounds validation (should be temporary…
tjwiebell fb7dc10
Remove obsolete test that is failing
tjwiebell f209db6
Make sure child component doesn't render until total pages is set
tjwiebell f44a9ad
Remove comment :upside_down_face:
tjwiebell c46296d
Merge branch 'develop' into feature/category-peregrine
devpatil7 b7f63ba
Add comment explaining loading indicator logic
tjwiebell File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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]; | ||
}; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
40 changes: 40 additions & 0 deletions
40
...-concept/src/RootComponents/Category/__tests__/__snapshots__/categoryContent.spec.js.snap
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
// Jest Snapshot v1, https://goo.gl/fbAQLP | ||
|
||
exports[`renders the correct tree 1`] = ` | ||
<article | ||
className="a" | ||
> | ||
<h1 | ||
className="b" | ||
> | ||
<div | ||
dangerouslySetInnerHTML={ | ||
Object { | ||
"__html": "test", | ||
} | ||
} | ||
/> | ||
<div /> | ||
</h1> | ||
<section | ||
className="c" | ||
> | ||
<Classify(Gallery) | ||
data={ | ||
Object { | ||
"id": 1, | ||
} | ||
} | ||
pageSize={6} | ||
title="test" | ||
/> | ||
</section> | ||
<div | ||
className="d" | ||
> | ||
<withRouter(Classify(Pagination)) | ||
pageControl={Object {}} | ||
/> | ||
</div> | ||
</article> | ||
`; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
171 changes: 68 additions & 103 deletions
171
packages/venia-concept/src/RootComponents/Category/category.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 | ||
tjwiebell marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}; | ||
|
||
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 ( | ||
<Query | ||
query={categoryQuery} | ||
variables={{ | ||
id: Number(id), | ||
onServer: false, | ||
pageSize: Number(pageSize), | ||
currentPage: Number(currentPage) | ||
}} | ||
> | ||
{({ loading, error, data }) => { | ||
if (error) return <div>Data Fetch Error</div>; | ||
// 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 ? ( | ||
<CategoryContent | ||
pageControl={pageControl} | ||
pageSize={pageSize} | ||
/> | ||
) : ( | ||
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 <div>Data Fetch Error</div>; | ||
// show loading indicator until our data has been fetched and pagination state has been updated | ||
if (!totalPages) return loadingIndicator; | ||
|
||
return ( | ||
<CategoryContent | ||
classes={classes} | ||
pageControl={totalWrapper} | ||
data={data} | ||
/> | ||
); | ||
}} | ||
</Query> | ||
); | ||
} | ||
} | ||
// if our data is still loading, we want to reset our data state to null | ||
return ( | ||
<CategoryContent | ||
classes={classes} | ||
pageControl={pageControl} | ||
data={loading ? null : data} | ||
/> | ||
); | ||
}; | ||
|
||
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; |
59 changes: 29 additions & 30 deletions
59
packages/venia-concept/src/RootComponents/Category/categoryContent.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 ( | ||
<article className={classes.root}> | ||
<h1 className={classes.title}> | ||
{/* TODO: Switch to RichContent component from Peregrine when merged */} | ||
<div | ||
dangerouslySetInnerHTML={{ | ||
__html: title | ||
}} | ||
/> | ||
<div className={classes.categoryTitle}>{categoryTitle}</div> | ||
</h1> | ||
<section className={classes.gallery}> | ||
<Gallery data={items} title={title} pageSize={pageSize} /> | ||
</section> | ||
<div className={classes.pagination}> | ||
<Pagination pageControl={pageControl} /> | ||
</div> | ||
</article> | ||
); | ||
} | ||
} | ||
return ( | ||
<article className={classes.root}> | ||
<h1 className={classes.title}> | ||
{/* TODO: Switch to RichContent component from Peregrine when merged */} | ||
<div | ||
dangerouslySetInnerHTML={{ | ||
__html: title | ||
}} | ||
/> | ||
<div className={classes.categoryTitle}>{categoryTitle}</div> | ||
</h1> | ||
<section className={classes.gallery}> | ||
<Gallery data={items} title={title} pageSize={pageSize} /> | ||
</section> | ||
<div className={classes.pagination}> | ||
<Pagination pageControl={pageControl} /> | ||
</div> | ||
</article> | ||
); | ||
}; | ||
|
||
export default classify(defaultClasses)(CategoryContent); | ||
export default CategoryContent; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.