Add useCategoryTree talon #1754
Conversation
jimbo
added
version: Patch
| } | ||
| }, [data, updateCategories]); | ||
| const mixinProps = useCategoryTree({ | ||
| categories, |
There was a problem hiding this comment.
I like that you're being explicit here about what you pass to the hook.
If your PR is missing information, check against the original template here. At a minimum you must have the section headers from the template and provide some information in each section. |
| const mixinProps = useCategoryTree({ | ||
| categories, | ||
| categoryId, | ||
| query: MENU_QUERY, |
There was a problem hiding this comment.
I think this is a good choice -- provide query via argument rather than importing within hook.
There was a problem hiding this comment.
Yeah, I think we have to let people customize the query or else these hooks will quickly lose value. That said, we'll have to find a way to set expectations for what kind of query should be passed in.
jimbo
force-pushed
the
jimbo/mixin-categorytree
branch
from
8e7add9 to
6db8de3
Compare
| @@ -0,0 +1,3 @@ | |||
| export { useCategoryBranch } from './useCategoryBranch'; | |||
There was a problem hiding this comment.
Once we're done with the peregrine refactors I'd like to have one PR where we go through and create these indexes and exports. Right now we're importing with full paths in venia-ui but we can clean all that up with proper exports.
| import { useCallback } from 'react'; | ||
|
|
||
| /** | ||
| * Returns props necessary to render a CategoryBranch component. |
There was a problem hiding this comment.
I love these comments. I'm going to go through and update my existing PRs with doc comments as well.
| const { onNavigate } = props; | ||
|
|
||
| const handleClick = useCallback(() => { | ||
| onNavigate(); |
There was a problem hiding this comment.
This isn't doing anything but wrapping/renaming the function. I think we don't need this talon.
There was a problem hiding this comment.
In general, a callback received as a prop shouldn't get directly applied as an event handler. I think it's important to wrap the event handler so that event doesn't get passed to onNavigate; onNavigate should be called with no arguments.
There was a problem hiding this comment.
OK -- in that case keep your eye out on my PRs cause I think I did not wrap these callbacks in some of my talons.
| return childCategories; | ||
| }, [categories, children]); | ||
|
|
||
| return { childCategories }; |
There was a problem hiding this comment.
I'm a big fan of not returning the pure data object of these query calls as I think I've done in some of my other talon PRs. This is good and I'll be using this going forward.
Description
Add a
useCategoryTreemixin to Peregrine, and use it in Venia'sCategoryTreecomponent.