-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Framework: Refactor PostTaxonomies to use withApiData
HoC
#2537
Conversation
@aduth Do you have any idea how to use |
You could have a parent component which manages the state, then passes the state as a prop to a child component, then available to This is precisely how search is implemented in Calypso's |
358550e
to
9fd630c
Compare
Can I get a 👍 or 👎? |
Codecov Report
@@ Coverage Diff @@
## master #2537 +/- ##
==========================================
+ Coverage 31.42% 31.95% +0.53%
==========================================
Files 177 177
Lines 5407 5576 +169
Branches 946 981 +35
==========================================
+ Hits 1699 1782 +83
- Misses 3135 3197 +62
- Partials 573 597 +24
Continue to review full report at Codecov.
|
super( ...arguments ); | ||
function PostTaxonomies( { postType, taxonomies, isOpened, onTogglePanel } ) { | ||
const availableTaxonomies = !! taxonomies.data | ||
? Object.values( taxonomies.data ).filter( ( taxonomy ) => taxonomy.types.indexOf( postType ) !== -1 ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: Lodash's filter
handles both undefined and Object types, so this could be simplified:
const availableTaxonomies = filter( taxonomies.data, ( { types } ) => includes( types, postType ) );
|
||
componentWillUnmout() { | ||
this.fetchTaxonomies.abort(); | ||
if ( ! availableTaxonomies.length ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should seek to avoid content flickering in and out of the sidebar as the page loads. Especially if we're more likely to have taxonomies than not, we could at least show the panel heading, even if the body can't be known yet. Alternatively, #2501 was designed for this purpose, and we could consider preloading taxonomies.
opened={ isOpened } | ||
onToggle={ onTogglePanel } | ||
> | ||
{ availableTaxonomies.map( ( taxonomy ) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To the above point, Lodash's reduce
similarly handles undefined
and object
types, so this could even manage the filtering and mapping in one, ensuring the panel heading is always shown.
<PanelBody
title={ __( 'Categories & Tags' ) }
opened={ isOpened }
onToggle={ onTogglePanel }
>
{ reduce( taxonomies.data, ( result, taxonomy ) => {
if ( includes( taxonomy.types, postType ) ) {
const TaxonomyComponent = taxonomy.hierarchical
? HierarchicalTermSelector
: FlatTermSelector;
result.push(
<TaxonomyComponent
key={ taxonomy.slug }
label={ taxonomy.name }
restBase={ taxonomy.rest_base }
slug={ taxonomy.slug }
/>
);
}
return result;
}, [] ) }
</PanelBody>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you know I prefer filter
+ map
over reduce
on small loops. I do understand the performance reasons for long loops.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant nothing of performance by my comment, filter
/ map
or whatever is fine, just that we can skip the early return and account for undefined
type with Lodash methods.
Just some more consistency in fetching API DATA