WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

#19815 closed task (blessed) (fixed)

Theme install and search screens should infinitely scroll

Reported by: helenyhou Owned by: nacin
Milestone: 3.4 Priority: normal
Severity: normal Version: 3.3
Component: Themes Keywords:
Focuses: Cc:

Description

As per scoping of 3.4, an improvement to the themes screens would be to have them infinitely scroll, especially now that they are floated divs instead of a table.

Pagination would be kept as-is for no-JS users. Loading and perhaps an end of results indicator also needed.

Attachments (14)

19815.3.diff (6.8 KB) - added by DH-Shredder 2 years ago.
Initial patch for adding inifinite scrolling to Themes screen (Currently Installed Only)
19815.4.diff (8.2 KB) - added by helenyhou 2 years ago.
Enables use on theme-install.php
19815.5.diff (7.3 KB) - added by helenyhou 2 years ago.
Hide pagination, move into theme.dev.js
19815.6.diff (7.0 KB) - added by DH-Shredder 2 years ago.
Spinner should function, added additional cleanup and caching window selector.
19815.7.diff (503 bytes) - added by helenyhou 2 years ago.
Only enqueue on theme-install search tab
19815.9.diff (7.4 KB) - added by garyc40 2 years ago.
More efficient throttling. Based on 19815.8.diff.
19815.9.2.diff (7.4 KB) - added by garyc40 2 years ago.
Removed some redundant variables that I added.
19815.8.diff (7.3 KB) - added by DH-Shredder 2 years ago.
Renamed class to ThemeScroller, caching more JQuery selectors, throttling scroll, using Deferred for AJAX calls, and general cleanup. Does not include 19815.7.diff [edit - get rid of stray propset]
19815.10.diff (7.9 KB) - added by DH-Shredder 2 years ago.
Combined garyc40's patch, and koopersmith's suggestions from IRC
19815.10.2.diff (8.2 KB) - added by DH-Shredder 2 years ago.
19815.10.diff, plus Fixed Paging, got rid of extra AJAX call for single page situations and end-of-pages, renamed maybeLoad to ajax, polling now clears when at end of content.
19815.11.diff (10.9 KB) - added by DH-Shredder 2 years ago.
Added JSON passing of necessary variables and removed parseQuery. Added $this->features to theme-install list table and removed $term global. Additional error handling.
19815.11.2.diff (10.1 KB) - added by DH-Shredder 2 years ago.
Refresh due to nacin's suggestions and updates
19815.12.diff (940 bytes) - added by DH-Shredder 2 years ago.
Be less paranoid about inputs. Removed some undefined checks per koopersmith. Still checking for initial input arrays.
19815.12.2.diff (1.8 KB) - added by DH-Shredder 2 years ago.
Yet more de-paranoidization

Download all attachments as: .zip

Change History (62)

comment:1 sabreuse2 years ago

  • Cc sabreuse@… added

comment:2 DH-Shredder2 years ago

  • Cc mike.schroder@… added

comment:3 azizur2 years ago

  • Cc azizur added

comment:4 follow-up: garyc402 years ago

I'd like to tackle this task if no core devs intend to take over yet? It's been a while since I last contributed something to core.

A question though, this infinite scroll enhancement can only be applied to the theme search screen right? Other screens such as Featured, Newest or Recently Updated don't have pagination, so I doubt wordpress.org has API for those screens.

comment:5 in reply to: ↑ 4 helenyhou2 years ago

Replying to garyc40:

I'd like to tackle this task if no core devs intend to take over yet?

DH-Shredder and I are teamed up on it (see http://wpdevel.wordpress.com/2012/01/11/this-is-not-a-feature-list/) but haven't quite started the dev cycle yet. Not sure how exactly we're approaching labeling who's on what, etc., but I imagine that doesn't exclude participation by any means :)

A question though, this infinite scroll enhancement can only be applied to the theme search screen right?

It also applies to the installed themes screen, which is where we are starting for proof-of-concept.

comment:6 follow-up: jane2 years ago

Infinite scroll on the themes screen has already been implemented on wordpress.com, so it may just be a matter of asking for their code and touching it up for core, rather than starting from scratch. Ask Koopersmith for details.

comment:7 in reply to: ↑ 6 DH-Shredder2 years ago

Replying to jane:

Infinite scroll on the themes screen has already been implemented on wordpress.com, so it may just be a matter of asking for their code and touching it up for core, rather than starting from scratch. Ask Koopersmith for details.

Have a tentative plan on how to implement if it does need to be from scratch, but will touch-base with Koopersmith to find out if that's an option, since it'd certainly speed things up.

comment:8 follow-up: garyc402 years ago

Looking forward to seeing your work on this :)

Several UX things I usually encounter with infinite scrolling in other web sites / apps:

  • As the list infinitely expands, the user scrolls farther away from the top. There should be some way for the user to quickly scroll to top to navigate to other pages, or refine their search criteria.
  • Let's say the user navigates away from the page. If the user decides to go back by clicking the browser's "Back" button, they should be presented with the previous theme browsing state, rather than with page 1.
  • Some sort of indication of the range of items being displayed versus the total number of items should be presented to the user as the next page is preloaded and displayed (e.g. 120 - 145 of 20000 total).

Don't know if all of these should apply to this case, but could be some idea to consider.

comment:9 in reply to: ↑ 8 helenyhou2 years ago

Replying to garyc40:

There should be some way for the user to quickly scroll to top to navigate to other pages, or refine their search criteria.

Hooray for #18578, then! Will keep the rest of your points in mind as we get on.

comment:10 scribu2 years ago

Related: #19469

DH-Shredder2 years ago

Initial patch for adding inifinite scrolling to Themes screen (Currently Installed Only)

comment:11 DH-Shredder2 years ago

Initial patch for adding infinite scrolling to Themes screen (Currently Installed Only).

This is a work in progress.

Enabled fetch-list for class-wp-themes-list-table.

Ported wp-includes/js/wplink.dev.js, which handles infinite scrolling for internal linking to wp-themes.dev.js for use in the Themes page.

Some things that are weird/known quirks:

  • There aren't any UI elements (i.e. spinners) to show that we're loading, but there are hooks in place to do this once the elements exist.
  • Labels are not modified based on "how many" we've loaded or how many are left. This means the paging always looks the same.
  • As soon as you've loaded all themes, it just doesn't load anymore. This is easy to hook into whatever we want to do when we're out of items.
  • For now at least, most of parseQuery was copied from tb_parseQuery (thickbox.js) into our JS for now, since making the change to thickbox.js would change how the thickbox handles query strings (whether more correct or not).
  • The parsing of the features[] query array is a bit hackish, in part due to how themes varies on how it includes it in the query string. Suggestions for better methods of parsing are welcome.
  • Yes, there is still some debug code, because it's a work in progress. This will be removed before final, of course, is marked FIXME, and should be obvious.

helenyhou2 years ago

Enables use on theme-install.php

comment:12 follow-up: helenyhou2 years ago

19815.4.diff gets it working on theme-install.php.

Some other issues:

  • Details do not work on Ajax-loaded items (needs delegation).
  • Initial search by filters is done via POST and so is currently not sending with the Ajax request.

comment:13 mikeschinkel2 years ago

Will inifinite scrolling be the only option? Personally I find infinite scrolling very disconcerting and would turn off where I can. JMTCW

comment:14 mikeschinkel2 years ago

  • Cc mikeschinkel@… added

comment:15 scribu2 years ago

  • Keywords ux-feedback added

In another case of "let's do it because it looks cool", there doesn't seem to be any UX rationale for this change posted anywhere.

The original idea seems to come from wordpress.com, which implemented infinite scroll on the themes screen. Ironically, it has now dropped this approach, in favor of simply listing all the existing themes on a single page.

One can argue that with infinite scroll, you can see more themes quicker. However, this can be addressed by increasing the number of themes listed per page: #19469

comment:16 jane2 years ago

[8:12pm] Jane: scribu: it was discussed at core team meetup

[8:12pm] scribu: ok, that doesn't really help

[8:12pm] scribu: it should be in trac or on wpdevel

[8:12pm] Jane: no, it doesn't

[8:12pm] Jane: yes, it should

[8:12pm] Jane: if i wasn't working 2.5 jobs for the salary of one, i would surely have finished collating and posting notes by now

[8:13pm] scribu: can't you get Matt to clone you or something?

[8:13pm] Jane: but this is going to be one of those things where it's a "the majority of leads wanted it"

[8:13pm] Jane: personally, i hate infinite scroll

[8:14pm] Jane: URLS4EVA

[8:14pm] scribu: I thought you were the UX lead

[8:14pm] Jane: yes, but we make decisions as a team

[8:14pm] Jane: and my hatred is personal, not objective

[8:14pm] Jane: which is why i agreed to it on the themes screen only

[8:15pm] Jane: where we could see how it did on a screen where it seemed likely to improve the experience rather than potentially degrade it

[8:15pm] scribu: *sigh*

[8:15pm] helenyhou: Jane: you are in my head. or i am in yours, whichever way.

[8:15pm] Jane: we've been talking about it in dev chats for almost two months now

[8:15pm] Jane: no one raised objections

[8:16pm] helenyhou: i think once all of the details are hidden by default and the grid is more even, it will look much better

[8:16pm] scribu: well, thanks anyway for responding, Jane

[8:16pm] Jane: sure

Version 0, edited 2 years ago by jane (next)

comment:17 scribu2 years ago

  • Keywords ux-feedback removed

comment:18 in reply to: ↑ 12 SergeyBiryukov2 years ago

Replying to helenyhou:

Initial search by filters is done via POST and so is currently not sending with the Ajax request.

#18094 has a patch for switching it to GET. Also related: #18724

helenyhou2 years ago

Hide pagination, move into theme.dev.js

comment:19 helenyhou2 years ago

19815.5.diff moves the JS into wp-admin/js/theme.dev.js on suggestion of rboren, hides .pagination-links, and ups the paging number for themes.php (see #19469). Still needs a loading spinner.

comment:20 ryan2 years ago

The last item in the original list is duplicated at the beginning of the first scroll fetch. This seems to happen only when using the feature filter and is true even when js is off. I suspect this has always been broken. Given that paging of search results is broken in 3.3, it is hard to tell. Regardless, not a blocker to landing .5.diff.

Last edited 2 years ago by ryan (previous) (diff)

comment:21 ryan2 years ago

JS notes from IRC discussion: Throttle the scroll event, or check the position of the window once every x milliseconds. Cache jQuery selectors. Replace delayedCallback with jQuery.Deferred.

DH-Shredder2 years ago

Spinner should function, added additional cleanup and caching window selector.

comment:22 DH-Shredder2 years ago

19815.6.diff adds:

  • Additional cleanup.
  • Spinner should function.
  • Cached window jQuery selector, primarily for maybeLoad -- others look to be cached already.

Still required, and coming next:

  • Throttle or timeout for checking of scroll position (per IRC)
  • Replace delayedCallback (per IRC)
  • Stop call (and spinner) from happening on "no more results"/bottom of page.
Last edited 2 years ago by DH-Shredder (previous) (diff)

comment:23 DrewAPicture2 years ago

  • Cc xoodrew@… added

comment:24 ryan2 years ago

In [19887]:

Infinite scroll for themes.php and theme-install.php. Bump per page limit for themes.php to 999. Props helenyhou, DH-Shredder. see #19815

comment:25 ryan2 years ago

In [19893]:

Infinite scroll for themes.php and theme-install.php. Bump per page limit for themes.php to 999. Props helenyhou, DH-Shredder. see #19815

comment:26 ryan2 years ago

[19887] was a miscommit of .5.diff. [19893] is .6.diff.

helenyhou2 years ago

Only enqueue on theme-install search tab

comment:27 helenyhou2 years ago

19815.7.diff makes sure the JS is only enqueued when needed on theme-install.php

comment:28 ryan2 years ago

Why wouldn't I want the other pages to infinite scroll?

comment:29 helenyhou2 years ago

They only return a limited number of results, and the infinite scroll seems to make them just keep repeating since there's no pagination to tell it to stop loading results.

comment:30 DH-Shredder2 years ago

In 19815.8.diff, renamed class to ThemeScroller, caching more jQuery selectors, throttling scroll, using Deferred for AJAX calls, and general cleanup.

Does not include 19815.7.diff, because this patch only modifies the JS involved.

Eyes on this patch welcome, since there are a significant amount of changes.
Alternate/preferred ways of throttling scroll also welcome!

Still to be done: Don't query to check if we already know that there aren't more items.

garyc402 years ago

More efficient throttling. Based on 19815.8.diff.

comment:31 garyc402 years ago

In 19815.8.diff, the throttle still doesn't work properly. To test this, simply add console.log('maybeLoad!'); at the beginning of the maybeLoad function. You'll see that it's called every single time the scroll event is triggered. Function call is expensive.

19815.9.diff has some improvement on the throttling method:

  • Since we're polling every xxx milliseconds anyways, there's practically no need for scroll event handler.
  • Threshold is checked right in the polling function, rather than in maybeLoad. This saves a function call every time we're polling.
  • scrollThrottleDelay is increased to 500.

garyc402 years ago

Removed some redundant variables that I added.

DH-Shredder2 years ago

Renamed class to ThemeScroller, caching more JQuery selectors, throttling scroll, using Deferred for AJAX calls, and general cleanup. Does not include 19815.7.diff [edit - get rid of stray propset]

DH-Shredder2 years ago

Combined garyc40's patch, and koopersmith's suggestions from IRC

comment:32 DH-Shredder2 years ago

Added 19815.10.diff, which contains 19815.8.diff plus garyc40's polling from 19815.9.2.diff, and koopersmith's suggestions from today's dev chat.

DH-Shredder2 years ago

19815.10.diff, plus Fixed Paging, got rid of extra AJAX call for single page situations and end-of-pages, renamed maybeLoad to ajax, polling now clears when at end of content.

comment:33 DH-Shredder2 years ago

Added 19815.10.2.diff: Includes changes from 19815.10.diff plus fixed paging cases where paging starts at > 1, got rid of extra AJAX call for single page situations and end-of-content, renamed maybeLoad to ajax, and polling setInterval now clears when at end of content.

comment:34 koopersmith2 years ago

In [19971]:

Second pass at infinite scroll for themes, including polling, fixed paging, and fewer ajax calls. props DH-Shredder, helenyhou, garyc40. see #19815.

comment:35 koopersmith2 years ago

I'd like to see a patch that creates the object with the query params in PHP and passes it to JS using json_encode. We'll be able to remove ~50 lines of JS (and the hackiness that is parseQuery).

DH-Shredder2 years ago

Added JSON passing of necessary variables and removed parseQuery. Added $this->features to theme-install list table and removed $term global. Additional error handling.

comment:36 DH-Shredder2 years ago

Attached 19815.11.diff

  • Added json_encode passing of necessary variables and removed parseQuery per koopersmith's suggestion. In the process, added $this->features to theme-install list table and removed $term global.
  • Additional error handling in theme.dev.js
  • Search in theme-install list table now runs strtolower on search argument to match the behavior in themes list table.

Eyes welcome on patch for general review or potential complications of the changes.

comment:37 nacin2 years ago

In [20048]:

In WP_Themes_List_Table, don't perform unnecessary sanitization on search terms or filter features. We only use these for case-insensitive comparison. see #19815.

DH-Shredder2 years ago

Refresh due to nacin's suggestions and updates

comment:38 DH-Shredder2 years ago

Attached 19815.11.2.diff:
Refresh of 19815.11.diff per nacin's suggestions and updates.

comment:39 nacin2 years ago

In [20094]:

Output themes and theme-install infinite scrolling args in JS, rather than parsing query strings. props DH-Shredder, helenyhou. Make WP_Theme_Install_List_Table extend WP_Themes_List_Table. Doesn't help much yet, but we should be able to dry things up further. see #19815.

comment:40 nacin2 years ago

In [20096]:

Use sanitize_key() for theme search tags/features. sanitize_title_with_dashes() is overkill. see #19815.

comment:41 nacin2 years ago

In [20097]:

Revert [20096] which was an accidental partial revert of [20094]. see #19815.

comment:42 nacin2 years ago

In [20098]:

Use sanitize_key(). sanitize_title_with_dashes() is more than we need. see #19815. see [20096], [20097].

comment:43 nacin2 years ago

In [20100]:

Allow WP_List_Table::_js_vars() to take an array of additional args to add. Allows us to have a single variable printing data when child classes need more data. Also, fix compact() call in [20094]. see #19815.

comment:44 nacin2 years ago

[20100] breaks the JS. koopersmith is cleaning it up and will account for the new variables.

comment:45 koopersmith2 years ago

In [20104]:

Partially revert [20100]. The list_args JS variable cannot be extended due to its use as an argument in fetch-list. see #19815.

We should reattempt extending the args created in WP_List_Table, but will need to deprecate the current list_args to do so.

Also, infinite scroll on themes pages is no longer broken. Go team.

DH-Shredder2 years ago

Be less paranoid about inputs. Removed some undefined checks per koopersmith. Still checking for initial input arrays.

comment:46 DH-Shredder2 years ago

19815.12.diff:
Be less paranoid about inputs. Removed some undefined checks per koopersmith. Still checking for initial input arrays. If/when the array inputs change, more checks can be removed.

DH-Shredder2 years ago

Yet more de-paranoidization

comment:47 DH-Shredder2 years ago

19815.12.2.diff:
Removed an additional check, and moved two member variable declarations, since both inputs now assume proper initialization.

comment:48 nacin2 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In [20242]:

Tidy up sanity checks in theme.dev.js. props DH-Shredder, fixes #19815.

Note: See TracTickets for help on using tickets.