Skip to content
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

Query: Remove 'inherit' override from query block attributes #67336

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

yogeshbhutkar
Copy link
Contributor

Fixes: #67288

What?

This PR removes the logic that tends to override the inherit value with the block's inherit value ( which is set to True by default ) leading to a bug where, if in the Pattern, we've specified inherit as false, then that gets overwritten to True which can potentially also ignore the perPage being set for the query block in the pattern. Better explained here: #67288 (comment)

How?

By removing the override of inherit, the bug was resolved.

Testing Instructions

  1. Create a Query Loop Block.
  2. Choose a Pattern titled List of posts, 1 column.
  3. Make sure to set the inherit to false in the Pattern for testing purposes.
  4. Notice the perPage is now whatever's mentioned in the perPage inside the pattern, instead of a default value from the reading settings. Earlier, even when inherit was explicitly set to false, the perPage from reading settings was used instead of using the perPage from the Block Code. ( Described in the above comment. )

Screenshots or screencast

Screen.Recording.2024-11-27.at.11.29.50.AM.mov

@yogeshbhutkar yogeshbhutkar marked this pull request as ready for review November 27, 2024 06:02
Copy link

github-actions bot commented Nov 27, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: yogeshbhutkar <[email protected]>
Co-authored-by: Mamaduka <[email protected]>
Co-authored-by: ntsekouras <[email protected]>
Co-authored-by: andreawetzel <[email protected]>
Co-authored-by: himanshupathak95 <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@Mamaduka Mamaduka added [Type] Bug An existing feature does not function as intended [Block] Query Loop Affects the Query Loop Block labels Nov 27, 2024
@Mamaduka
Copy link
Member

Based on the method comment, I think the behavior is intentional. Variations and patterns need to specify if they're not using an inherit query.

@ntsekouras, can you firm this?

@yogeshbhutkar
Copy link
Contributor Author

yogeshbhutkar commented Nov 27, 2024

Based on the method comment, I think the behavior is intentional. Variations and patterns need to specify if they're not using an inherit query.

@ntsekouras, can you firm this?

That's what I thought too. But even if the inherit value is set to false, it gets overwritten to true, and this in turn overrides the perPage already set in the pattern to a more generic value set in reading settings or defaults to just 10.

I think the perPage if declared inside the pattern must be prioritised over the more generic reading settings.

Or perhaps, we can use this approach:

block.attributes.query = {
  ...block.attributes.query,
  postType,
  inherit: block.attributes.query.inherit ?? inherit,
};

Here, if the inherit value is omitted altogether, we can use the default inherit value of the query block.

Please let me know which direction to proceed with so that I can make the changes accordingly in the proposed PR.

@ntsekouras
Copy link
Contributor

ntsekouras commented Dec 20, 2024

Thanks for the PR! I think it makes sense to preserve the inherit value (as is in trunk), since the pattern should be more about presentation and not changing such values. I may be misunderstanding the issue here but are you having a problem when you have a custom(inherit:false) Query Loop and choose a pattern which also contains a custom one?

Just noting some recent updates that might be relevant here:

  1. the default inherit value changed
  2. some logic was introduced inside an effect to enforce inherit value in some cases.

I'll cc @mikachan as well for any thoughts.

@andreawetzel
Copy link
Contributor

The change to the default value of inherit is an issue for patterns that include multiple queries such as the Twenty Twenty Four pattern Grid of posts featuring the first post, 2 columns because the second query is intended to have an offset number to match the queried post number from the first query.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Query Loop Affects the Query Loop Block [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Query Loop does not use theme's pattern perPage value for number of posts
4 participants