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

Support for automatically iterating paginated resources in core-data #6180

Closed
danielbachhuber opened this issue Apr 14, 2018 · 26 comments
Closed
Assignees
Labels
[Feature] Document Settings Document settings experience [Priority] High Used to indicate top priority items that need quick attention REST API Interaction Related to REST API [Type] Enhancement A suggestion for improvement.

Comments

@danielbachhuber
Copy link
Member

danielbachhuber commented Apr 14, 2018

In cases like #4622, #4022, #5820 and others, UI dependent on withAPIData can break when all available items aren't loaded in the UI.

For example, editor/components/post-author/check.js will only fetch up to 100 users:

/wp/v2/users?context=edit&per_page=100

If there are more than 100 users, the UI currently only displays the first 100.

Because it's a valid expectation to bring all items into the DOM (and not depend on autocomplete or similar), it would be helpful if core-data supported automatically iterating paginated resources to bring the entire dataset within scope.

@danielbachhuber danielbachhuber added [Type] Enhancement A suggestion for improvement. [Feature] Document Settings Document settings experience REST API Interaction Related to REST API labels Apr 14, 2018
@danielbachhuber danielbachhuber added this to the Merge Proposal: REST API milestone Apr 14, 2018
@danielbachhuber danielbachhuber self-assigned this Apr 14, 2018
@danielbachhuber
Copy link
Member Author

@WordPress/gutenberg-core Could I get some direction on how you'd like to handle this? It appears hierarchical-term-selector.js isn't even using withAPIData to fetch its data.

I see editor/components/post-author/check.js is beginning to use withSelect but not (yet?) for author data itself: https://github.com/WordPress/gutenberg/pull/6167/files#diff-4305c6ef4b86dbe9d7b1db481b038b98R35

@noisysocks
Copy link
Member

Because it's a valid expectation to bring all items into the DOM (and not depend on autocomplete or similar)

Playing devil's advocate: What if there are 1000 users? 10,000 users? Is making dozens of API requests when the sidebar opens acceptable?

I'd prefer that we build e.g. a generic component that replaces<select> and supports paginating through large datasets.

@danielbachhuber
Copy link
Member Author

Playing devil's advocate: What if there are 1000 users? 10,000 users? Is making dozens of API requests when the sidebar opens acceptable?

Two points:

  1. WordPress already lists 1000 authors if you have them, so it wouldn't be a departure from existing behavior.
  2. I'm not sure 1000 authors is a majority use-case we should take on additional UI work (i.e. introducing pagination, etc.) for at this point.

@gziolo
Copy link
Member

gziolo commented Apr 16, 2018

We are slowly deprecating withApiData in favor of wp.data and select/withSelect in particular. I thing @aduth and @youknowriad should help to give directions to this one. I think @aduth was looking into something similar last week with tags, where it wasn't possible to pick a tag on sites where there were more than 500 tags.

@danielbachhuber danielbachhuber changed the title Support for automatically iterating paginated resources in withAPIData Support for automatically iterating paginated resources in core-data Apr 18, 2018
@danielbachhuber danielbachhuber added the [Priority] High Used to indicate top priority items that need quick attention label Apr 18, 2018
@aduth
Copy link
Member

aduth commented Apr 18, 2018

I don't think we want to continuously load in the background for resources which may or may not ever be needed for how the user plans to interact with the current editor session.

For something like the author selector, I see it working as a component which loads an initial set of options, then more only in response to:

  • As the user scrolls or selects through the list and nears the extent of known options, fetch the next page
  • If the user searches for a specific value, query and fetch the value

There are a few side considerations here:

  • The default field value should always be known, even if not within an otherwise initial set (e.g. assigned author may not be included in first call to /wp/v2/users)
  • The server will let us know (via X-WP-Total response header) the total number of resources. We can use this to simulate a more accurate scrollable size

You can see some of these ideas in action here: https://bvaughn.github.io/react-virtualized/#/components/InfiniteLoader

Other prior art:

As far as generalization, I agree concepts of pagination and querying should be built-in to core-data. There's some related discussion here in #6085. Ultimately I think we could create some higher-order reducers to encapsulate common pagination / querying behaviors (related article).

Other prior art:

@danielbachhuber
Copy link
Member Author

For something like the author selector, I see it working as a component which loads an initial set of options, then more only in response to:

What accessibility concerns are there with this approach? cc @afercia

You can see some of these ideas in action here: https://bvaughn.github.io/react-virtualized/#/components/InfiniteLoader

If this proves to be significantly greater level of effort than simply loading all of the user and term data into scope, would you be open to doing the simple implementation first?

@aduth
Copy link
Member

aduth commented Apr 18, 2018

It will be a significantly greater level of effort 😄 As for iterations, only my standing hesitation of contentment with mediocrity.

@danielbachhuber
Copy link
Member Author

As for iterations, only my standing hesitation of contentment with mediocrity.

I understand. As it exists now though, the current implementation is a blocking bug. Furthermore, I'm concerned there are unknown accessibility issues with the lazy-load approach. I don't know the full history of why Select2 has been so difficult to introduce.

@aduth
Copy link
Member

aduth commented Apr 18, 2018

I might imagine if it's considered an in-place substitute for <select>, there are issues in that pressing e.g. "T" to navigate to an author option "Thomas" would not work as expected if we haven't yet loaded these resources. My thinking would be that there is substitute behavior for this by incorporating the search field, where such an interaction would shift focus back to the search field and trigger the search†.

† Also makes me wonder if we should have such behavior when starting to enter text while navigating listed block options in the inserter.

@aduth
Copy link
Member

aduth commented Apr 18, 2018

As for iterations, only my standing hesitation of contentment with mediocrity.

I understand. As it exists now though, the current implementation is a blocking bug.

Sure, this is precisely how mediocre happens. There's a sense of urgency when a thing is completely unusable, as exists here today. This urgency is lost when a "not good, but tolerable" solution is implemented. This is a trade-off, of course; the simplest solution is often more immediately achievable. But it leaves a good probability that it will remain this way forever. I am not saying one way or the other is "right", and is dependent on the circumstances.

I think this is the time where someone chimes in with a favorite mantra of lazy development "perfect is the enemy of good". 😉

@danielbachhuber
Copy link
Member Author

Sure, this is precisely how mediocre happens.

Just so it's stated, I'm not going to enter this debate.

@pento
Copy link
Member

pento commented Apr 19, 2018

Obviously, paginating through the output of these endpoints is a less than an ideal solution.

I'd love to see a lazy loader solution for this, but there have been significant challenges in the past. As @danielbachhuber Select2 was investigated, but it had significant accessibility issues.

Since then, the Woo team have created SelectWoo, which has a focus on accessibility, so may be worth exploring.

That said, investigating either of these will require a significant amount of work to ensure they're a good long term option, I would place this firmly as a stretch goal for 5.0, at best.

In the mean time, I have no problem solving this by automatically iterating over paged endpoints. Particularly with the preloading support added in #6076, I wouldn't expect this to add any significant load to the server, beyond what the Classic Editor already does.

@aduth
Copy link
Member

aduth commented Apr 24, 2018

Reiterating my own points from today's discussion in core-editor Slack:

There are multiple things going on in parallel here. I’m not trying to solve them all at once. Things I see off the top of my head:

  • We have queryable data, which should be implemented in core-data with a generalized solution across the various resources available. This is what I’m working on.
  • We have a number of components which have their own ad-hoc ways of fetching data, such as the author selector. These would need to be ported to take advantage of core-data.
  • We have a UI which needs to be present all options in an accessible/usable and bandwidth-efficient manner
    So updating the author UI to infinitely load options is not strictly in conflict with what I am working on, though may become redundant as a decision can be reached on an ideal UI (or not). Generally speaking, I’m personally motivated to not be loading dozens of pages of data in the background for a UI which may or may not ever be touched by the user in their session. This is not within my immediate scope, but could benefit from it.

I’m also wondering if the infinite loading is as simple as it appears on the surface; how do you inform the user that options are still loading when you’ve only loaded 5 of the 35 API pages of data? Is it prone to the same accessibility issues described in #5921 (comment) with infinite scroll and recalculating the number of options?

@afercia
Copy link
Contributor

afercia commented Apr 25, 2018

Since then, the Woo team have created SelectWoo, which has a focus on accessibility, so may be worth exploring.

The accessibility team already tested it a few months ago, we were pinged by the developer working on it. Although there are some improvements, it's still not sufficient from an accessibility perspective, especially when it comes to dynamically adding new sets of results.

I’m also wondering if the infinite loading is as simple as it appears on the surface; how do you inform the user that options are still loading when you’ve only loaded 5 of the 35 API pages of data?

I won't repeat the a11y issues already described on #5921 (comment) but yeah, infinite scrolling is also an usability issue. It gives the illusion to solve a problem, but are we really sure it really does it? A good UI should just give users the necessary information to operate on the UI, not lead to a sense of loss of control, and not cause frustration.

I see infinite scrolling as something that developers like more than users 🙂 It's a pattern that was vastly experimented in the past but today a quick search for infinite scrolling usability gives tons of articles highlighting its weak points. The fact is, infinite scrolling is not for all users, some will love it others will hate it. There are alternatives. Pagination is the most obvious, and there's an example pattern in WordPress (though it would need a better design):

screen shot 2018-04-25 at 10 11 03

@pento
Copy link
Member

pento commented Apr 26, 2018

Thanks for the extra feedback, @afercia!

The variations of this problem are fairly obviously quite difficult, I don't believe that an infinite scroll solution is viable (or necessary) for merge.

After chatting to @danielbachhuber earlier, I believe that a combination of things will provide a reasonable fix:

  • When a user is logged in, (and perhaps a higher role than subscriber, to prevent abuse on open registration sites) allow them to specify per_page=-1, so all of the data is retrieved in one request.
  • As mentioned in Abysmal response time when fetching pages to be listed in page attributes #5376, improve the REST API's _fields parameter, to reduce processing on fields that aren't being returned, improving server response time.
  • Audit our use of _fields. In the case of the /users request, it retrieves the entire user object, when we really just need the id and name. This will significantly reduce bandwidth requirements.

@kadamwhite
Copy link
Contributor

I'll take a swing at what it would take to support -1 between now and the weekend, and post with whatever I find then. In the REST API meeting today @TimothyBJacobs raised concerns about the behavior of per_page=-1 on very, very large sites, but this approach still feels the most pragmatic and we can cross the scale bridge once we come to it.

@Lewiscowles1986
Copy link
Contributor

Lewiscowles1986 commented Apr 28, 2018

I see infinite scrolling as something that developers like more than users

Would an adequate search/input like an autocomplete not be a more developer friendly solution than paging over data? Doesn't React have an autocomplete frontend component? If there is a component for autocomplete, then is it not a case of defining a custom render & fetch method? (I clearly need to find more time to play with it)

Iterating would take orders of magnitude more time on sites where for example customer roles are tied to users (WooCommerce). The success of the site would almost become it's downfall if the only pattern for access, means scanning the entire database (even if only in small chunks). It's equivalent to a full-table-scan in database vs an index-lookup.

I Have a customer that has quite a small e-commerce site. They still have > 2500 users (most are customers), which even with a pagination size of 100 entries is 25 scroll interactions, it's 250 at a page size of 10.

I've just tested using the following bookmarklet / console scripts

Add a page every second

setInterval(function() { wp.apiRequest({method:'POST',path:'/wp/v2/pages',credentials:true,data:{'title':'REST-gen-'+Date.now(),'content':'empty','status':'publish'}}) }, 1000)

visiting /wp-admin/edit.php?post_status=publish&post_type=page I can see it creates 500+ pages, all published

visiting /wp-admin/post-new.php?post_type=page and running the following content script

Count pages in side-bar of Gutenberg parent page

[].slice.call(document.querySelectorAll('#inspector-select-control-0 option')).length

shows 101 (no matter how many pages I add.

Fundamentally I'd expect the UI patterns and user-experience for a site with 2500 entries for a dropdown to be different to that of a smaller database with dozens, or hundreds of records to avoid lots of visual scanning over what would be from this image quite busy.

Sites that are even larger, I'd imagine would hit a point, where there were too many pages or posts.

WordPress already lists 1000 authors if you have them, so it wouldn't be a departure from existing behavior.

I was talking about the existing behaviour with @mikelittle at WordCamp London RE: sites with a lot of pages and the pages dropdown in admin (similar issue as getting all users with > 1000) is the page sidebar, where you can select a parent page, which at present from above tops-out at 101 in gutenberg for top-level pages.

It's a problem I've encountered in other software projects. I'll admit I'm lazy up-front, so I always reach for an auto-complete, with select semantics (double-click searches and needs a special case). I'm not sure how it would work with React or WP_Query server-side (mine uses jQuery UI Autocomplete), but I've never had a complaint (scanning is moved to the DB).

@Lewiscowles1986
Copy link
Contributor

Lewiscowles1986 commented Apr 28, 2018

As an alternative to only infinite loading, what about skip to specific page using prev-next and a random- direct paging access input box? (please excuse the fonts, and probably imagine an "of {totalPages})

WordPress Posts Paging Alternative

It doesn't rule-out an infinite loader as well as manual prev-next and paging controls, but the numerical selection could become a browser native number box with minimum, maximum.

@karmatosed
Copy link
Member

karmatosed commented Apr 30, 2018

@Lewiscowles1986 whilst a possibility I think we need to avoid such complicated visual interfaces as you suggest. That's a lot of things going on in one place. This is where combining autocomplete and infinite scroll, when done well (there are numerous bad examples) can hit the right spot.

It's worth noting autocomplete and suggestions have accessibility benefits, for example those with dyslexia. Where we can limiting the actions you need to do and suggesting ultimately helps everyone.

@Lewiscowles1986
Copy link
Contributor

@karmatosed wondering what you think of the above direct paging access?

Infinite scroll sounds like it's got legs here, I'm trying to understand why besides being able to provide access to all records.

@afercia
Copy link
Contributor

afercia commented Apr 30, 2018

Autocomplete / suggestions and infinite scroll are two very different things. While the first can be made accessible and there are already examples in WordPress and Gutenberg, infinite scroll can't be made accessible for the reasons already explained in #5921 (comment)

@karmatosed
Copy link
Member

@Lewiscowles1986 I think what I am seeing above is confusing as an interface. There's a lot going on there. For example hierarchy, is paging a higher priority than the content itself? How would I also know what page information would be on to add a page number?

Autocomplete / suggestions and infinite scroll are two very different things.

They are but combined they can resolve issues of usability with infinite scroll for example. I am talking not about accessibility in saying that as that obviously is another strong factor to iterate here.

@Lewiscowles1986
Copy link
Contributor

Indeed my efforts were mostly as an alternative to the paging via links above.

How would I also know what page information would be on to add a page number?

I didn't quite understand that. It's not direct access based upon knowledge of page numbers to resources, but a way to bypass iterating over a result set.

Say I click next.

  • Page n+1 no difference to infinite scroll
  • If however it becomes obvious 1 or more pages in I can skip ahead an arbitrary number of result sets.
  • Best case it can have some wins for large sets. It's no good for small sets or alone (what is?)

Im not 100% behind it myself, but I use it

@karmatosed
Copy link
Member

I didn't quite understand that. It's not direct access based upon knowledge of page numbers to resources, but a way to bypass iterating over a result set.

In the graphic above the number is an input field, that's why I asked how would you know what input to add. I get there is pagination but if it's an input field that indicates that someone would/could add a number.

@danielbachhuber
Copy link
Member Author

The original aim of this issue is no longer valid:

Support for automatically iterating paginated resources in core-data

The more immediate problems have been resolved with #6627 #6657 #6722

We'll bring per_page=-1 to core with https://core.trac.wordpress.org/ticket/43998

I'm open to discussing UI/UX improvements in a new issue if someone cares to forward a proposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Document Settings Document settings experience [Priority] High Used to indicate top priority items that need quick attention REST API Interaction Related to REST API [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

No branches or pull requests

10 participants