#50105 closed defect (bug) (fixed)
Remove infinite scrolling behavior from the Media grid
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 5.8 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Media | Keywords: | has-screenshots needs-testing needs-dev-note |
Focuses: | ui, accessibility, javascript | Cc: |
Attachments (23)
Change History (160)
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
5 years ago
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
5 years ago
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
5 years ago
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
5 years ago
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
5 years ago
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
5 years ago
This ticket was mentioned in Slack in #accessibility by nrqsnchz. View the logs.
5 years ago
#11
@
5 years ago
Related: #50410 which is a blocker for this ticket, at least for the part concerning the text with the media items count / total.
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
5 years ago
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
5 years ago
#14
@
5 years ago
50105.2.diff refreshes the patch and:
- adds a "Jump to first new item" button
- the button is only displayed on focus for keyboard users
- uses more
wp.i18n
This ticket was mentioned in Slack in #accessibility by audrasjb. View the logs.
5 years ago
#16
@
5 years ago
- Keywords needs-dev-note needs-testing added
Looks really good to me!
Adding needs-dev-note
and needs-testing
keywords.
#17
@
5 years ago
See screenshot above. To clarify the behavior:
- the "Load more" button is only visible when there are more items to load
- the spinner is only visible when items are being fetched
- the "Jump to first new item" button is only visible on focus for keyboard users, after new items have been added
Still to do [Edited]:
- avoid focus loss when the "Load more" button disappears because there are no new items to load
- scrolling issues in the media modal
- update the number of total attachments when adding / removing attachments: an alternative option could be hiding the counts after attachments add/removal
- consider whether the default "per page" of 40 attachments should be increased
- what to do with other long-standing issues made more apparent by this patch, for example: when filtering attachments, see #50025 (Filters don't show new uploads)
This ticket was mentioned in Slack in #core-media by pbiron. View the logs.
5 years ago
This ticket was mentioned in Slack in #core-media by antpb. View the logs.
5 years ago
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
5 years ago
This ticket was mentioned in Slack in #core by afercia. View the logs.
5 years ago
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
5 years ago
#24
@
5 years ago
@afercia The dependent tickets are still open. Is this still a viable option in 5.5 at this point?
#25
@
5 years ago
- Keywords needs-dev-note removed
@davidbaumwald I don't think this ticket can make it for 5.5. I didn't punt it yet because I wanted the component maintainers to be aware of this ticket.
This ticket was mentioned in Slack in #core by david.baumwald. View the logs.
5 years ago
#27
@
5 years ago
- Milestone changed from 5.5 to 5.6
Since this has been discussed at a media in depth, it's safe to say the Media team is aware of it and just has not had time to circle back. Going to punt due to lack of movement so far.
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
4 years ago
#29
@
4 years ago
- Milestone changed from 5.6 to Future Release
This ticket was discussed during today's accessibility bug-scrub: given the Beta 1 period is approaching fast and considering this ticket would need greater visibility and a good amount of time for in depth testing, the teams agreed to punt it to future release.
This ticket was mentioned in Slack in #core-media by joedolson. View the logs.
4 years ago
This ticket was mentioned in Slack in #core-media by antpb. View the logs.
4 years ago
#33
@
4 years ago
- Milestone changed from Future Release to 5.7
As mentioned here: https://core.trac.wordpress.org/ticket/50025#comment:13
#50025 is a blocker for this so we should prioritize it before this ticket, but both seem well in scope of 5.7 as good focuses. Moving to the milestone to maintain visibility. :)
This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.
4 years ago
#37
@
4 years ago
- Keywords needs-refresh removed
Patch refreshed. This is unchanged from the last stages that Andrea was working on; I think that there are a couple things that could be improved, and the 'todo' items from https://core.trac.wordpress.org/ticket/50105#comment:17 are still to be done.
Noting: 'Jump to first new item' is only in tab sequence when moving forward, and not when moving backwards from the media item sidebar. It's only meaningful right after new items have been loaded, however, so it's a legitimate question whether it should even continue to be present once it's used.
This ticket was mentioned in Slack in #accessibility by alexstine. View the logs.
4 years ago
#39
@
4 years ago
- Keywords needs-testing-info added
In preparation for Testing Scrub scheduling, please provide more information for the testers:
- What are the steps to reproduce the problem?
- Are there any testing dependencies such as a plugin or script?
- What is the expected behavior after applying the patch?
This ticket was mentioned in Slack in #core-media by joedolson. View the logs.
4 years ago
#41
@
4 years ago
After the feature was implemented, it would be good to be able to turn it on or off.
There are users who prefer the infinite scrolling.
This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.
4 years ago
This ticket was mentioned in Slack in #accessibility by sarahricker. View the logs.
4 years ago
This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.
4 years ago
This ticket was mentioned in Slack in #core-media by antpb. View the logs.
4 years ago
This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.
4 years ago
#47
@
4 years ago
50105.5.diff refresh against trunk.
This ticket was mentioned in PR #979 on WordPress/wordpress-develop by adamsilverstein.
4 years ago
#48
Trac ticket: https://core.trac.wordpress.org/ticket/50105
#49
@
4 years ago
@ricjcs
After the feature was implemented, it would be good to be able to turn it on or off.
There are users who prefer the infinite scrolling.
This is achievable with the patch using a filter (media_library_infinite_scrolling
); one code line or a mini plugin can be used to re-enable this.
adamsilverstein commented on PR #979:
4 years ago
#50
#51
@
4 years ago
This is close; the ticket still needs a little work to be completely ready though, see https://core.trac.wordpress.org/ticket/50105#comment:37
Since we are in 5.7 beta, we need to get this in soon or punt to 5.8.
#52
@
4 years ago
avoid focus loss when the "Load more" button disappears because there are no new items to load
What is the desired behavior? should the focus be set to the last media item or to the "Jump to first new item" button?
scrolling issues in the media modal
Not certain what these are, any more details?
update the number of total attachments when adding / removing attachments: an alternative option could be hiding the counts after attachments add/removal
Ideally these counts should stay synced with the collection total.
consider whether the default "per page" of 40 attachments should be increased
40 feels ok for now. We can make sure this is filterable. It would be nice if this were user controllable like posts per page, or terms per page are from the screen options tab. We would need to integrate this into the modal itself though since it operates in numerous contexts.
#53
@
4 years ago
Updated patch:
1) Changes button style from hero to normal size
2) Places buttons on a single line
3) Makes 'jump to first new item' visible by default.
This fixes a scrolling issue where navigating to the 'Jump' link could cause an unnecessary change in the view position, and makes the 'Jump' link available to sighted mouse users. Since the first added item is not obvious, this is useful for all users.
4) Visual highlight on first newly added item, to make easier to see what's new.
What is the desired behavior?
In my opinion, move focus to the 'Jump to first new item'; this is more predictable in my opinion.
5) Moves focus to 'Jump to first new item' button when Load more button does not exist in final screen.
6) Updates @since references to 5.7.0, optimistically
Update the number of total attachments.
This will require some kind of adaptation to getTotalAttachments. Seems like the model doesn't refresh when items are added or removed, though the collection does.
40 feels ok for now.
Agreed. Possibly a filter now, then explore adding a UI enhancement to support changing it later?
This ticket was mentioned in Slack in #core-media by antpb. View the logs.
4 years ago
#55
@
4 years ago
- Keywords early added
- Milestone changed from 5.7 to 5.8
It was agreed in the recent Media component meeting by participants that it might be too late in the 5.7 cycle to feel comfortable changing the media library experience. It was determined that this is better fit as an early
candidate for 5.8 and should be committed asap.
Still pending as @joedolson noted in the meeting The major outstanding items: update attachments count, review design, add filter for attachment number.
This ticket was mentioned in Slack in #core-media by david.baumwald. View the logs.
4 years ago
This ticket was mentioned in Slack in #core-media by joedolson. View the logs.
4 years ago
This ticket was mentioned in Slack in #core-media by joedolson. View the logs.
4 years ago
#59
@
4 years ago
- Keywords needs-refresh added
@joedolson Do you think you'll have time to take another look at this for 5.8?
This ticket was mentioned in Slack in #core by chaion07. View the logs.
4 years ago
This ticket was mentioned in Slack in #core-media by hellofromtonya. View the logs.
4 years ago
#62
@
4 years ago
- Keywords needs-refresh removed
Patch refreshed & new screenshots loaded showing new version of load more design & visual indicator of first new loaded attachment.
This ticket was mentioned in Slack in #core by chaion07. View the logs.
4 years ago
This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.
4 years ago
This ticket was mentioned in Slack in #core by chaion07. View the logs.
4 years ago
This ticket was mentioned in Slack in #core-media by antpb. View the logs.
4 years ago
#67
@
4 years ago
Updated patch accomplishes four things:
1) Language to 'Jump' link changed based on Media meeting.
2) Language describing what's currently shown changed.
3) Mis-alignment of 'Jump to first loaded' button when all items displayed fixed.
4) Added highlight on all items added to help visual users perceive the insertion point and change.
@antpb I didn't find a way to parse over the newly added items, but I did find that I could add a class to all the items already loaded, and use a :not selector to invert the selection for highlighting.
#68
@
4 years ago
Still needed: update total collection size if items are added or deleted from the collection.
This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.
4 years ago
#70
@
4 years ago
Tested
- MacOS 11.3
- Safari 14.1
- WordPress 5.8-alpha-50427-src
- Twenty Twenty-One 1.3
After applying it, you have to runnpm run dev
to get the expected behavior, might be worth adding it to the testing instructions.
The patch applies almost cleanly. The total counter doesn't update during the upload of the images in grid mode. Only after you refresh the page or do another upload. I am going to attach the image.
That said, I have some feedback:
- UX/UI - There are no screen options in the grid view, so you can not change the number of images loaded in one go
- Copy - The instruction "Jump to first loaded" is not clear. First what? The image of course, but could be clearer. First when? In the last batch. What if I want to jump to the first image loaded in the penultimate batch?
- UX/UI - Have you considered pagination? If you have a large number of images, this patch solves the infinite scrolling issues but doesn't address the issue of separating the images in smaller batches.
Thanks!
#71
@
4 years ago
Thanks, @francina!
In a media meeting, we discussed that making the number of images a setting would probably be a future release issue, as there are a number of UI questions to resolve for that. That's also true for pagination, as implementing pagination into the media library would be a much more substantial change to the way it works.
The total counter updating is still an unresolved issue; it's the major outstanding item to figure out.
The language for the 'jump to' is a hard problem. It can't be 'image', since the media library contains attachments of all types. It could be 'attachment', I suppose; I'm not sure how readily understood that would be. But there's a question of how verbose it makes sense to be, e.g. "Jump to first attachment loaded in this request" seems awfully verbose.
I have no particular bias about the specific language; happy to hear suggestions! The aim is certainly to reach something that will be easily understood, but preferably also concise.
Can you tell me what in the patch didn't apply cleanly for you? This patch should be clean; it was fresh yesterday, so I'm not sure why it wouldn't apply cleanly.
I'm not sure what npm run dev
does; it's not a command I've ever used.
This ticket was mentioned in Slack in #core-test by francina. View the logs.
4 years ago
#73
@
4 years ago
Thanks @joedolson!
Makes sense to ship this as an MVP and then iterate with pagination or any other solution that makes this accessibile, easly understandable and usable 🙌
Jump to first attachment loaded in this request" seems awfully verbose.
I'll take awfully verbose over unclear anytime, but this is only my opinion of course. "Request" is a technical term, end-users will not relate to it probably.
"Jump to the first file uploaded in the previous XX" where XX could be "operation, load, command" perhaps?
I am also trying to think about how that would sound in Italian "Vai al primo file caricato dopo l'ultima operazione".
I am adding a reminder to myself to ping copywriters and polyglots to come up with a reasonable text :)
Regarding the patch, it applies cleanly. What I meant by "almost" referred to known bug of the total counter. But I wasn't clear on communicating that, my bad.
And finally the npm command. This ticket doesn't have testing instructions, hence the needs-testing-info
keyword added.
Since WordPress 5.6, the release squad has a testing focus lead: it was observed that not many tickets have clear testing instructions added, in the form of a list of steps needed.
I did figure out what to do and noticed that when you use wordpress-develop
you need an additional npm run dev
after the patch is applied. But it is only for people that test with that specific environment.
This ticket was mentioned in Slack in #core-media by antpb. View the logs.
4 years ago
#75
@
4 years ago
It was discussed in the recent Media component meeting that saying "Jump to first loaded item" is a great way to make this not focus on images or any other specific media type and still be clear. I agree that some folks might be confused and miss the "loaded" part of that phrase but it may be the clearest option to start with. This is also a very low impacting button to press as it keeps you on the same page and only changes your focus. Folks will learn pretty quickly what it does.
@hellofromTonya mentioned in the meeting on the pending count issue, "I think it would be a better experience for users if we could solve it before 5.8 beta. That said, it would be good to get the patch committed and then work on that specific issue as a follow-up."
It was agreed by multiple participants in the meeting that landing this sooner and iterating is ideal. :)
This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.
4 years ago
#77
@
4 years ago
- Keywords commit added; needs-testing-info removed
Updated patch for commit.
Until commit, this will require you to build WordPress after applying the patch.
Testing:
Testing preconditions: rquires a minimum of 41 items in media to interact with these controls.
- throttle the network via your browser's dev tools: this helps in observing the UI behavior e.g. the spinner visibility
- test without and with media trash enabled (by setting define( 'MEDIA_TRASH', true ); in your wp-config.php file)
- test the filter add_filter( 'media_library_infinite_scrolling', 'return_true' ); to restore the infinite scrolling behavior
- test the Media Library
- test the Media modal with Classic Editor (Add Media, featured image)
- test the Media modal with Gutenberg (image, gallery, featured image...)
- optionally: test the Media modal with plugins that implement custom frames to upload / select attachments
- inspect the queries array of cached collections
#81
@
4 years ago
This change has broken in the Unsplash plugin. It is likely this change will break other plugins that extend the media library. Is there anyway of doing this, without breaking integrations?
#82
@
4 years ago
We'd need some information about what the breakage is to know; can you provide any documentation on what has broken?
The plugin author should be able to restore infinite scroll if they need to by using the new filter for that:
add_filter( 'media_library_infinite_scrolling', 'return_true' )
But if there's anything we can do to ensure plugins still work, happy to work on that.
This ticket was mentioned in Slack in #core by sergey. View the logs.
4 years ago
This ticket was mentioned in Slack in #core-media by antpb. View the logs.
4 years ago
#85
@
4 years ago
The issue here is the change of the output from the function wp_ajax_query_attachments
$result = array(
'attachments' => $posts,
'totalAttachments' => $query->found_posts,
);
wp_send_json_success( $result );
The output admin ajax call is now different. I think changing the format of the response here will likely break other plugin as this admin ajax call is used in other plugin, not just core. I have used in a couple of plugins I have used for clients.
Instead of changing the output of the admin ajax call, not just use the REST API for these views. If you add the _envelope param to the rest api call, then you would get the same data. See https://make.wordpress.org/core/wp-json/wp/v2/media?_envelope=true
#86
@
4 years ago
- Keywords commit removed
Good spot. However, switching to the REST API for these views will also break plugins that override this output, for example Asset Manager Framework.
How about adding the totalAttachments
data as an HTTP header and leaving the body response unchanged?
#87
@
4 years ago
- Keywords commit added
Whoops, I didn't see that this had already been committed :)
#88
@
4 years ago
- Keywords needs-dev-note 2nd-opinion added; has-patch needs-testing early commit removed
- Resolution fixed deleted
- Status changed from closed to reopened
Reopening for further discussion of the above. There are several plugins that override this Ajax response.
If this change stays as-is then this will need a dev note about the breaking change. If not then I think delivering the total via an HTTP header makes sense, but this might also mean that core cannot trust that the header is in place if a plugin overrides the Ajax handler.
#89
@
4 years ago
There maybe other plugins out there that are using these reusing underscore views but do not override the admin ajax calls. In the Unsplash plugin for example, we have a custom REST API endpoint, the replicates the format of the admin ajax call.
Currently the view does this.
parse: function( response, xhr ) {
if ( ! _.isArray( response.attachments ) ) {
response = [ response.attachments ];
}
this.totalAttachments = parseInt( response.totalAttachments, 10 );
However, as documented above, the properties attachments and totalAttachments may not exist. The code should check first to see if these properties exist, if, they do not fall back to the previous behavior. If totalAttachments does not exist, infinite scroll should also be re-enabled.
Something like this might work.
parse: function( response, xhr ) {
if ( response.hasOwnProperty('attachments') && ! _.isArray( response.attachments ) ) {
response = [ response.attachments ];
}
if ( response.hasOwnProperty('totalAttachments') ) {
this.totalAttachments = parseInt( response.totalAttachments, 10 );
}
#90
@
4 years ago
If we use http headers, I believe we should follow the headers in the REST API.
Example
$response->header( 'X-WP-Total', (int) $total_posts );
$response->header( 'X-WP-TotalPages', (int) $max_pages );
#91
@
4 years ago
I think it makes sense to check for the existence of values specific to this update and make the change conditional on their existence. If any plug-in is overriding the data, then I think we should assume that plug-in is taking full responsibility for how that data is fetched and displayed.
That makes a lot of sense to me, at least.
This ticket was mentioned in Slack in #core-media by joedolson. View the logs.
4 years ago
This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.
4 years ago
This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.
4 years ago
This ticket was mentioned in PR #1335 on WordPress/wordpress-develop by adamsilverstein.
4 years ago
#95
- Keywords has-patch added
Trac ticket:
#96
@
4 years ago
- Keywords has-patch removed
I looked at this a little bit (PR in progress: https://github.com/WordPress/wordpress-develop/pull/1335) and found the xhr object is not passed back to fetch as expected here, so I was unable to extract the post count from the header. Perhaps we are overriding fetch somewhere else?
Appreciate any help getting the header here, I agree moving this data to the header will be best to avoid breaking anything. Another option would be to introduce a new endpoint for the request.
#97
@
4 years ago
There's a lot of overriding happening in \wp\api.js
; that's where the X-WP-Nonce header is set and fetched.
It's interesting that the XHR object documented there is not, in fact, an XHR object, and I can't see anything that's overriding fetch on the model. Sync is being extended, but that shouldn't be changing anything about the fetch.
Might be able to follow the path used for adding nonces to get this header. I'm currently looking at that.
#98
@
4 years ago
OK. Here's what I'm seeing, I think.
The Media library overrides backbone.sync in media\models\query.js
.
Backbone uses .fetch to get data using .sync, so this is overriding the default Backbone return value, using wp.media.ajax
instead. wp.media.ajax
(an alias of wp.ajax.send
) doesn't return an XHR object, but instead returns a promise.
We could probably pass an option into wp.media.ajax
that gets the XHR object, though I'm not at all sure what additional consequences that could have.
In my quick test, I was able to get an XHR object, but I didn't see the header showing up, but I ran out of time today to try to track down why.
#99
@
4 years ago
I wondered not getting the xhr object back was related to the jQuery update and something changing under the hood there but that doesn't seem to be the case.
Once we get headers, I still recommend using the same headers as the REST API .
X-WP-Total
X-WP-TotalPages
Not adding new response headers, will help compatibility with those using firewalls and reserve proxies. New headers have to added to allowed lists in these cases. Reusing an existing header, avoids this.
#100
@
4 years ago
@adamsilverstein's incomplete patch uses X-WP-TotalPages, so that's covered @spacedmonkey. I don't see any reason we'd change that.
#101
@
4 years ago
Updated, functional patch: https://github.com/WordPress/wordpress-develop/pull/1335, attaching here as well.
#102
@
4 years ago
In 50105.10.diff:
- Pass attachment totals back in headers, restoring original response data shape.
- Use a customized fetch handler to capture the returned X-WP-Total header.
- Ensure spinner hides after load completes.
- Use approach from rest endpoint to calculate correct X-WP-Total & X-WP-TotalPages headers.
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
4 years ago
#104
@
4 years ago
We are leaving this open through Beta 1 as this is partially merged or related to a partially merged feature. All that is needed is a review of the above patch and potentially refinement. If the patch can't be merged before Beta 3 we'll need to revert to allow time for the revert to be tested before Release Candidate.
#105
@
4 years ago
- Keywords needs-testing added
@spacedmonkey can you confirm the latest patch fixes your use case in the Unsplash plugin?
@johnbillion in the latest patch, i reverted the ajax callback data return shape so that it should match what existed in 5.7, putting the post and page count information in the header as you suggested. Can you confirm this would restore compatibility for Asset Manager Framework?
@joedolson Can you please review the latest patch and give it a test? This biggest potentially breaking change is setting the xhr as context so we have access to the headers. I also wound up changing how the spinner hiding is handled to fix the behavior.
Once we confirm this works well we should commit before beta 2 to get wider testing, especially with potentially affected plugins.
#106
@
4 years ago
This seems to work fine for core.
For the Unsplash plug-in, it partially works - Unsplash is replacing the returned object, however, so the XHR object isn't where we expect it to be. We can easily work around that for this one use case, since they retain the original response in _more
:
if ( this.hasOwnProperty( 'getResponseHeader' ) ) { collection.totalAttachments = parseInt( this.getResponseHeader( 'X-WP-Total' ), 10 ); } else { collection.totalAttachments = parseInt( this._more.getResponseHeader( 'X-WP-Total' ), 10 ); }
Probably not practical for core, though.
I also tried setting a default value if we couldn't get the total attachments.
if ( this.hasOwnProperty( 'getResponseHeader' ) ) { collection.totalAttachments = parseInt( this.getResponseHeader( 'X-WP-Total' ), 10 ); } else { collection.totalAttachments = 0; }
That worked really well for Unsplash, and I can imagine it might be a good general solution, though we'd only find that out in real-world testing.
I did find a bug that occurs when changing tabs - on the initial load of the Media Library tab, the load more button appears, but then it's gone if you switch away (e.g. to the Upload files tab or to a custom tab, like Unsplash.) Exploring this.
#107
@
4 years ago
Ah - the load more button isn't gone, it's just hidden.
Problem is that the updateLoadMoreView
is only loaded on the 'add remove reset' events, and those aren't fired when switching tabs.
updateLoadMoreView
needs to fire when the view is shown, as well.
This is fixed by running updateLoadMoreView on the same expectations as updateContent
, which makes sense to me.
New patch coming shortly.
@
4 years ago
Handles returned data that isn't an XHR object & creates load more view when switching tabs in modal
#108
@
4 years ago
New patch handles a couple more edge cases; could still use feedback from @spacedmonkey and @johnbillion
#109
@
4 years ago
I am currently doing some testing. I am not seeing the load more button, so I will hold off.
The unsplash intergration is going to be updated, as there is a new wrapping div and the css has changed. I think changing to use the headers, does not things a lot easier for us to update. We also have to map the header.
There needs to be some error handling to check if the header exists. If it doesn't exist fallback to the old behaviour.
#110
@
4 years ago
Test the most recent patch, if you're not already.
The load more button should not show up in Unsplash.
#111
@
4 years ago
After doing some more testing, I got it working. I am going to need to do a small release to disable infinite scroll and make some css tweaks, but the latest patch work well!
This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.
4 years ago
#113
@
4 years ago
Regarding the new wrapper - that was needed in order to group the media items and the load more interface. I think that is needed, but it predates the point where I was working on this issue, so I can't say for sure that it's the only path to do this.
But it sounds like you can work around it pretty easily, which really should generally be true.
Noting that the additional wrapper should go into the dev note on this.
#114
@
4 years ago
- Keywords commit added; 2nd-opinion needs-testing removed
I've done some continuing testing, and I think this is ready for commit. It clearly fixes issues with the existing implementation, creates a path for plugins to better adapt for the changes, and as far as I can find, creates no new issues.
#118
@
4 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
It looks like [51145] introduced an undefined variable. $query_args
should be $query
in wp_ajax_query_attachments()
.
I double checked with @adamsilverstein just to be sure, and it seems this does need to be corrected.
#119
@
4 years ago
- Keywords needs-patch added; commit removed
Indeed, $query_args
is not defined, and $query
should be used instead.
#123
@
4 years ago
Personally I didn't really like the implementation of this feature and the usability. To scroll through multiple images I have to press the button many times, it's not as fast as before. I think it should be an optional feature.
This ticket was mentioned in Slack in #core-media by azaozz. View the logs.
4 years ago
#126
@
4 years ago
- Keywords needs-dev-note added
Hi, @wildworks
How did you implement the filter? I just tested using the Media Library Enable Infinite Scroll plug-in, and it seemed to work as expected.
I was able to reproduce the JS error when media_library_infinite_scrolling is enabled, however, and I'll look into that.
#127
@
4 years ago
JS error is pretty straightforward; will get that fixed shortly. Created new ticket: #53672
#128
@
4 years ago
@joedolson
Thank you for the quick response.
I will test it again as soon as the new version is released.
This ticket was mentioned in Slack in #core-test by francina. View the logs.
4 years ago
This ticket was mentioned in Slack in #core-test by boniu91. View the logs.
4 years ago
This ticket was mentioned in Slack in #core-test by lucatume. View the logs.
4 years ago
#132
@
4 years ago
Hey! I do have some questions after testing this ticket.
- It looks like RWD when adding a image from the post editing view is not that good:
- I'm not sure what's the purpose of the grey background under newly loaded images, is this expected?
- I experienced situation when the first and second newly uploaded images are not highlighted (note that the first image is in the dotted frame)
#133
@
4 years ago
40 feels ok for now. We can make sure this is filterable. It would be nice if this were user controllable like posts per page, or terms per page are from the screen options tab.
@adamsilverstein A filter was not added for 5.8? If not would it make sense to open a trac ticket for it? I am personally most interested in a filter, though a UI would be nice for normal users.
#134
@
4 years ago
I think that @antpb was going to do that (or at least open a ticket for discussing changes to the perpage number for the media gallery) following up from the media meeting last week, but it hasn't been opened yet. No filter was added in 5.8, so I think you could go ahead and open that ticket.
This ticket was mentioned in Slack in #core-media by antpb. View the logs.
4 years ago
#136
@
4 years ago
The fix for this ticket seems to have introduced a backwards-incompatible change to the query-attachments
ajax action.
Previously it wasn't required to send a posts_per_page
variable. Now not sending this causes a division by 0 to happen.
Note that not passing it causes it to fallback to the global option but this is done on the query_vars
property of the query and not on the query
property which would still contain the originally passed parameters.
In this fix the query
property is used to determine the total pages which may not have the posts_per_page
set at all.
50105.diff is a proof of concept meant for initial testing.
Still to do:
It would be great to run extended testing on different installations, with a varying mount of attachments stored in the media library. I'd suggest to focus on:
Suggestions for testing:
define( 'MEDIA_TRASH', true );
in yourwp-config.php
file)add_filter( 'media_library_infinite_scrolling', '__return_true' );
to restore the infinite scrolling behaviorqueries
array of cached collectionsAccessibility:
For now, clicking the "load more" sets focus on the first added attachment. I think it would be best to explore other options, as moving focus unexpectedly should preferably be avoided.
Questions:
1
The patch changes the data returned by the WP_Query used to retrieve attachments. Previously, the response only held the array of attachments. Now, it holds 2 items:
found_posts
Later, the response is parsed so that it passes the attachments array to the views and sets a new property for the number of total attachments. Basically, the response and the cached
queries
use a different format. An interesting question in this regard is: should the old format be kept when themedia_library_infinite_scrolling
filter restore infinite scrolling? The media grid works regardless but I guess this should be explored for potential backwards compatibility issues?2
One more interesting question for anyone familiar with the history of the media views is what are the various "compat" methods meant for? This part is a bit obscure to me. See things like:
Attachment.saveCompat
save-attachment-compat
and relatedwp_ajax_save_attachment_compat
attachment_fields_to_edit
things (I think this was used for the old upload method?)