Make WordPress Core

Opened 12 years ago

Closed 12 years ago

Last modified 7 years ago

#19815 closed task (blessed) (fixed)

Theme install and search screens should infinitely scroll

Reported by: helenyhou's profile helenyhou Owned by: nacin's profile 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 kirasong 12 years ago.
Initial patch for adding inifinite scrolling to Themes screen (Currently Installed Only)
19815.4.diff (8.2 KB) - added by helenyhou 12 years ago.
Enables use on theme-install.php
19815.5.diff (7.3 KB) - added by helenyhou 12 years ago.
Hide pagination, move into theme.dev.js
19815.6.diff (7.0 KB) - added by kirasong 12 years ago.
Spinner should function, added additional cleanup and caching window selector.
19815.7.diff (503 bytes) - added by helenyhou 12 years ago.
Only enqueue on theme-install search tab
19815.9.diff (7.4 KB) - added by garyc40 12 years ago.
More efficient throttling. Based on 19815.8.diff.
19815.9.2.diff (7.4 KB) - added by garyc40 12 years ago.
Removed some redundant variables that I added.
19815.8.diff (7.3 KB) - added by kirasong 12 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 kirasong 12 years ago.
Combined garyc40's patch, and koopersmith's suggestions from IRC
19815.10.2.diff (8.2 KB) - added by kirasong 12 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 kirasong 12 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 kirasong 12 years ago.
Refresh due to nacin's suggestions and updates
19815.12.diff (940 bytes) - added by kirasong 12 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 kirasong 12 years ago.
Yet more de-paranoidization

Download all attachments as: .zip

Change History (63)

#1 @sabreuse
12 years ago

  • Cc sabreuse@… added

#2 @kirasong
12 years ago

  • Cc mike.schroder@… added

#3 @azizur
12 years ago

  • Cc azizur added

#4 follow-up: @garyc40
12 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.

#5 in reply to: ↑ 4 @helenyhou
12 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.

#6 follow-up: @jane
12 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.

#7 in reply to: ↑ 6 @kirasong
12 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.

#8 follow-up: @garyc40
12 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.

#9 in reply to: ↑ 8 @helenyhou
12 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.

#10 @scribu
12 years ago

Related: #19469

@kirasong
12 years ago

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

#11 @kirasong
12 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.

@helenyhou
12 years ago

Enables use on theme-install.php

#12 follow-up: @helenyhou
12 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.

#13 @mikeschinkel
12 years ago

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

#14 @mikeschinkel
12 years ago

  • Cc mikeschinkel@… added

#15 @scribu
12 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

#16 @jane
12 years ago

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

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

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

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

[8:12pm] Jane__: yes, it should[[BR]]

[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[[BR]]

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

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

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

[8:14pm] Jane__: URLS4EVA[[BR]]

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

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

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

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

[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[[BR]]

[8:15pm] scribu: *sigh*[[BR]]

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

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

[8:15pm] Jane__: no one raised objections[[BR]]

[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[[BR]]

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

[8:16pm] Jane__: sure
Last edited 12 years ago by scribu (previous) (diff)

#17 @scribu
12 years ago

  • Keywords ux-feedback removed

#18 in reply to: ↑ 12 @SergeyBiryukov
12 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

@helenyhou
12 years ago

Hide pagination, move into theme.dev.js

#19 @helenyhou
12 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.

#20 @ryan
12 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 12 years ago by ryan (previous) (diff)

#21 @ryan
12 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.

@kirasong
12 years ago

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

#22 @kirasong
12 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 12 years ago by kirasong (previous) (diff)

#23 @DrewAPicture
12 years ago

  • Cc xoodrew@… added

#24 @ryan
12 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

#25 @ryan
12 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

#26 @ryan
12 years ago

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

@helenyhou
12 years ago

Only enqueue on theme-install search tab

#27 @helenyhou
12 years ago

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

#28 @ryan
12 years ago

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

#29 @helenyhou
12 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.

#30 @kirasong
12 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.

@garyc40
12 years ago

More efficient throttling. Based on 19815.8.diff.

#31 @garyc40
12 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.

@garyc40
12 years ago

Removed some redundant variables that I added.

@kirasong
12 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]

@kirasong
12 years ago

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

#32 @kirasong
12 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.

@kirasong
12 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.

#33 @kirasong
12 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.

#34 @koopersmith
12 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.

#35 @koopersmith
12 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).

@kirasong
12 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.

#36 @kirasong
12 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.

#37 @nacin
12 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.

@kirasong
12 years ago

Refresh due to nacin's suggestions and updates

#38 @kirasong
12 years ago

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

#39 @nacin
12 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.

#40 @nacin
12 years ago

In [20096]:

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

#41 @nacin
12 years ago

In [20097]:

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

#42 @nacin
12 years ago

In [20098]:

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

#43 @nacin
12 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.

#44 @nacin
12 years ago

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

#45 @koopersmith
12 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.

@kirasong
12 years ago

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

#46 @kirasong
12 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.

@kirasong
12 years ago

Yet more de-paranoidization

#47 @kirasong
12 years ago

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

#48 @nacin
12 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.

#49 @afercia
7 years ago

In 41019:

Administration: in WP_List_Table->pagination(), properly concatenate CSS classes for the pagination links when infinite_scroll is set to true.

Props SGr33n.
See #19815.
Fixes #40003.

Note: See TracTickets for help on using tickets.