WIP: adding new featured content block #625
Conversation
iruzevic
added
new block
blocks/init/src/Blocks/components/load-more/assets/load-more.js
Outdated
Show resolved
Hide resolved
blocks/init/src/Blocks/custom/featured-content/components/featured-content-options.js
Outdated
Show resolved
Hide resolved
blocks/init/src/Blocks/custom/featured-content/components/featured-content-options.js
Outdated
Show resolved
Hide resolved
| global $post; | ||
|
|
||
| $args = [ | ||
| 'post_type' => $featuredContentPostType, | ||
| 'posts_per_page' => $featuredContentLayoutTotalItems, | ||
| 'fields' => 'ids', | ||
| ]; | ||
|
|
||
| if ($featuredContentTaxonomy) { | ||
| $args['tax_query'][0] = [ | ||
| 'taxonomy' => $featuredContentTaxonomy, | ||
| 'field' => 'id', | ||
| ]; | ||
|
|
||
| if ($featuredContentTerms) { | ||
| $args['tax_query'][0]['terms'] = array_map( | ||
| function ($item) { | ||
| return $item['value']; | ||
| }, | ||
| (array)$featuredContentTerms | ||
| ); | ||
| } elseif ($featuredContentUseCurrentTerm && $post instanceof WP_Post && !$featuredContentServerSideRender) { | ||
| $currentTerms = get_the_terms($post->ID, $featuredContentTaxonomy); | ||
|
|
||
| if ($currentTerms) { | ||
| $args['tax_query'][0]['terms'] = [$currentTerms[0]->term_id]; // @phpstan-ignore-line | ||
| } | ||
| } else { | ||
| $args['tax_query'][0]['operator'] = 'NOT IN'; // phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_tax_query | ||
| } | ||
| } | ||
|
|
||
| $excludeList = []; | ||
|
|
||
| if ($featuredContentExcludeCurrentPost && $post instanceof WP_Post) { | ||
| $excludeList[] = $post->ID; | ||
| } | ||
|
|
There was a problem hiding this comment.
It would be much better if we could have this inside a class method. Ideally this will be a filter that takes multiple arguments (as WP_Query args) and then just returns the query object in $mainQuery variable.
There was a problem hiding this comment.
agreed but this will overcomplicate the implementation so I would like to not do that.
There was a problem hiding this comment.
You could have a Filters folder with different filter classes that could handle this kind of logic. The implementation would just then be as in other projects (just apply_filter calls).
There was a problem hiding this comment.
i have opened an issue #631
| } | ||
|
|
||
| if ($excludeList) { | ||
| $args['post__not_in'] = $excludeList; |
There was a problem hiding this comment.
This should be handled inside the WP_Query loop. We'd check if current post ID is same as the one in the loop and just continue; with the loop
There was a problem hiding this comment.
💯
We should also check the performance impacts that the tax queries are having on huge post numbers.
There was a problem hiding this comment.
agreed, but with this approach pagination is a nightmare so In general there max 1-2 items here if any so lets not overcomplicate this for a minor optimisation.
There was a problem hiding this comment.
We'll need to really be careful about this because there are projects that have over 10k posts, and this could be a bottleneck.
There was a problem hiding this comment.
agreed, i have created an issue for this #632
|
|
||
| const layoutUse = checkAttr('layoutUse', attributes, manifest); | ||
| const layoutType = checkAttr('layoutType', attributes, manifest); | ||
| const layoutTotalItems = checkAttr('layoutTotalItems', attributes, manifest); |
There was a problem hiding this comment.
This naming is a bit odd, the label says Maximum items to show, would it be better to name it layoutMaxNumberOfItems?
There was a problem hiding this comment.
i like the keep the attribute naming as short and simple as possible. The label is here for the users. So i think this one is ok
blocks/init/src/Blocks/components/layout/components/layout-options.js
Outdated
Show resolved
Hide resolved
|
|
||
| // Prepare body data. | ||
| const body = { | ||
| method: 'POST', |
There was a problem hiding this comment.
Fetching implies GET, not POST 😬
We should use appropriate HTTP methods for things we do with the API (or AJAX in this case).
There was a problem hiding this comment.
The problem here is that I send the data from frontend to backend via json format because we have multiple decoded params that can't be sent via get parameter. So this is why I was using POST
There was a problem hiding this comment.
I think we discussed this yesterday, no?
There was a problem hiding this comment.
yep this comment was from earlier than that :D this is resolved
blocks/init/src/Blocks/custom/featured-content/components/featured-content-options.js
Outdated
Show resolved
Hide resolved
blocks/init/src/Blocks/custom/featured-content/components/featured-content-options.js
Outdated
Show resolved
Hide resolved
blocks/init/src/Blocks/custom/featured-content/components/featured-content-options.js
Outdated
Show resolved
Hide resolved
blocks/init/src/Blocks/custom/featured-content/featured-content.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Left some questions and suggestions. Like the direction where this is going, but the PR is encompassing a bit more than just a featured content block.
For instance, I'd like for the layout component to be extracted in a separate PR for easier review.
You can always create branches, cherry pick, create a separate PR, then once that is merged to develop branch, just rebase the current PR. Keeps things a bit easier to review 😉
* develop: adding variations to store updating change log tweak column chart icon add new block icons and update existing indent correction in css variables fix ssr render, indent fix for ssr remove console log from css-variables js Fix a bug in Safari for hamburger animations Adds media-breakpoints variable that pulls variables from global manifest.json add base font size calc docs, add multiple breakpoints for base font size, removed font-size in editor styles, add frontend specific font size multiplied by 100% add useRemBaseSize to manifest, add store manipulations of useRemBaseSize, add useRemBaseSize to manifest schema, add functionality to block generated css variables
Co-authored-by: Igor Obradović <[email protected]>
Co-authored-by: Denis Žoljom <[email protected]> Co-authored-by: Igor Obradović <[email protected]>
…block-featured-content * commit '0c042623c825cb01a49abe09d51bb771d6544af4': Apply suggestions from code review Update blocks/init/src/Blocks/components/load-more/docs/readme.mdx # Conflicts: # blocks/init/src/Blocks/components/layout/docs/readme.mdx
|
Once the tests are written just squash merge this to have a cleaner commit history. |
* develop: adding new featured content block (#625) adding postcss transforms fixing issue # Conflicts: # .storybook/preview.js # blocks/init/storybook/preview.js
Description
TODO:
Linked PR
This is a linked PR with libs: infinum/eightshift-libs#297