Renamed & moved QueryControls from blocks/QueryPanel to components/QueryControls #5319
Conversation
components/query-controls/index.js
Outdated
| } ) { | ||
| return [ | ||
| ( onOrderChange || onOrderByChange ) && ( | ||
| ( onOrderChange && onOrderByChange ) && ( | ||
| <SelectControl | ||
| key="query-panel-select" |
There was a problem hiding this comment.
There are 3 key props that still reference panel. All of them should start now with: query-controls-*.
There was a problem hiding this comment.
Nice catch keys were updated.
| const termsTree = buildTermsTree( categories ); | ||
| return ( | ||
| <TreeSelect | ||
| { ...{ label, noOptionLabel, onChange } } |
| import TreeSelect from '../tree-select'; | ||
|
|
||
| export default function CategorySelect( { label, noOptionLabel, categories, selectedCategory, onChange } ) { | ||
| const termsTree = buildTermsTree( categories ); |
There was a problem hiding this comment.
Just wanted to leave a note that it is suboptimal to compute this on every prop change. Whenever selectedCategory changes this will also need to be processed. @aduth, does it make sense to use memoize here to have this logic executed only when the list of categories changes?
There was a problem hiding this comment.
I think it is a really nice suggestion maybe we can simply add memoize to buildTermsTree function.
blocks/library/latest-posts/block.js
Outdated
| { ...{ order, orderBy } } | ||
| numberOfItems={ postsToShow } | ||
| categories={ get( categoriesList, 'data', {} ) } | ||
| category={ categories } |
There was a problem hiding this comment.
Shouldn't category be renamed to selectedCategoryId in here to reflect its further usage? Should we also rename categories to something that is closer to the aforementioned prop name?
There was a problem hiding this comment.
The props were renamed :)
|
@jorgefilipecosta thanks for working on it. Great work so far. I had some comments regarding |
gziolo
added
the
[Feature] Extensibility
jorgefilipecosta
force-pushed
the
update/exposed-QueryControls-component
branch
from
2d983da to
da4156a
Compare
…QueryControls. This change also exposes the component to be available for general usage. A depency of applyWithAPIData was also removed from the component, because the component should not be the one dealing with data requests.
jorgefilipecosta
force-pushed
the
update/exposed-QueryControls-component
branch
from
da4156a to
b446fc0
Compare
| { ...{ order, orderBy } } | ||
| numberOfItems={ postsToShow } | ||
| category={ categories } | ||
| categoriesList={ get( categoriesList, 'data', {} ) } |
There was a problem hiding this comment.
categoriesList will never be undefined, get isn't strictly necessary.
In a perfect world we wouldn't assume the source of the value, but given the data property access, we already kinda are.
|
Thank you for landing this PR @gziolo! |
Closes #4998.
This change also exposes the component to be available for general usage, the export was missing. A dependency of applyWithAPIData was also removed from the component because the component should not be the one dealing with data requests.
This PR comes as a result of discussions that happened in #4999 (comment).
How Has This Been Tested?
Verify that QueryControls currently only used in latest posts works as expected. E.g: change the sorting, category, and number of items and verify everything works as expected.