Set the media attachments query posts per page conditionally. - #12838
Set the media attachments query posts per page conditionally.#12838afercia wants to merge 1 commit into
Conversation
|
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 Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
There was a problem hiding this comment.
Pull request overview
Adjusts the media attachments query pagination defaults so the number of items requested per query matches the UI mode (infinite scroll vs. “Load more”) in the media library experience.
Changes:
- Updates
defaultArgs.posts_per_pageto 40 as the baseline default. - Conditionally sets
args.posts_per_pageto 80 inQuery.get()when infinite scrolling is disabled.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if ( wp.media && wp.media.view && wp.media.view.settings && wp.media.view.settings.infiniteScrolling === 0 ) { | ||
| args.posts_per_page = 80; | ||
| } |
There was a problem hiding this comment.
I believe this point is valid. For example, the media gallery applies perPage-1, which could unintentionally overwrite this.
Furthermore, I believe that checking for infiniteScrolling === 0 is too strict. This is because it may lead to mismatches if media_view_settings filters set the value to false or "0", as these do not align with the strict equality check. See https://github.com/t-hamano/wordpress-develop/blob/57b8740a0a9b0f65333ff0b6aaa9c5ff66232f89/src/js/media/views/attachments/browser.js#L80
There was a problem hiding this comment.
I second this. It's very important to preserve any perPage / posts_per_page that override the defaults.
| // Fill any other default query args. | ||
| _.defaults( args, Query.defaultArgs ); | ||
|
|
||
| // When 'Load more' is enabled, increase the posts pr page to 80. |
| if ( wp.media && wp.media.view && wp.media.view.settings && wp.media.view.settings.infiniteScrolling === 0 ) { | ||
| args.posts_per_page = 80; | ||
| } |
There was a problem hiding this comment.
I second this. It's very important to preserve any perPage / posts_per_page that override the defaults.
| _.defaults( args, Query.defaultArgs ); | ||
|
|
||
| // When 'Load more' is enabled, increase the posts pr page to 80. | ||
| if ( wp.media && wp.media.view && wp.media.view.settings && wp.media.view.settings.infiniteScrolling === 0 ) { |
There was a problem hiding this comment.
I believe we should use ! wp.media.view.settings.infiniteScrolling instead of wp.media.view.settings.infiniteScrolling === 0 to match the existing convention as the rest of the UI.
| // Fill any other default query args. | ||
| _.defaults( args, Query.defaultArgs ); | ||
|
|
||
| // When 'Load more' is enabled, increase the posts pr page to 80. |
|
I'm all for improvements and I will leave it in your hands. There are some performance related considerations in the two related tickets: but I'm not seeing a final word on ti. |
We can take over the task if necessary, but what is the reason you cannot continue working on it? |
I'm not comfortable with answering your question publicly on this issue. |
I have no idea why you're taking that attitude, but I'm going to move this PR forward anyway. |
Trac ticket: https://core.trac.wordpress.org/ticket/65564, https://core.trac.wordpress.org/ticket/65850
Sets the
posts_per_oagearg conditionally in the media attachments browser based on whether Infinite scrolling is enabled (40) or the 'Load more' button is enabled instead (80),Note that the
defaultArgs.posts_per_pageis meant to be static so the logic to set theposts_per_pagevalue conditionally is added later where it is used, in theget()method.Use of AI Tools
None
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.