adobe / react-spectrum

A collection of libraries and tools that help you build adaptive, accessible, and robust user experiences.
https://react-spectrum.adobe.com
Apache License 2.0
13.04k stars 1.13k forks source link

useAsyncList can't load more after a client side sort action is performed #1847

Closed LFDanLu closed 2 weeks ago

LFDanLu commented 3 years ago

๐Ÿ› Bug Report

If the user performs list.sort with useAsyncList, the cursor is wiped from the data tracker. This means subsequent list.loadMore calls won't trigger load since they are early returned if data.cursor is null.

๐Ÿค” Expected Behavior

loadMore calls should still work even if list is sorted.

๐Ÿ˜ฏ Current Behavior

loadMore calls don't work if list is sorted. It may work if the sorting happens. Probably works fine for server side sorting because we wrap that up in the load function instead of having a separate sort function.

๐Ÿ’ Possible Solution

My first thought is that we should fall back to data.cursor here if action.cursor doesn't exist: https://github.com/adobe/react-spectrum/blob/main/packages/@react-stately/data/src/useAsyncList.ts#L162. Might need to sort again after the loadMore finishes so that the items are in proper sort order still

๐Ÿ’ป Code Sample

Try the http://localhost:9003/?path=/story/table--async-loading story. Sort it first and then try to scroll to load more.

๐ŸŒ Your Environment

Software Version(s)
react-spectrum . 3.9.0
Browser
Operating System

๐Ÿงข Your Company/Team

RSP

๐Ÿ•ท Tracking Issue (optional)

LFDanLu commented 3 years ago

Discussed with team, recommendation is that users shouldn't be using async loading + client side sorting. Task here is to add something in the docs to warn people away from doing this. Will need to update some of the other doc examples to not do this.

flayner2 commented 2 years ago

Discussed with team, recommendation is that users shouldn't be using async loading + client side sorting. Task here is to add something in the docs to warn people away from doing this. Will need to update some of the other doc examples to not do this.

But what about integrating with a Table component, for example? This is exactly my use case: I do client side sorting when the user clicks on the column headers of a Table, but at the same time the table must be paginated because my dataset is quite large. I handle the loadMore() call with an onClick event on a button at the bottom of the table, but after a sort the button doesn't work anymore. Server-side sorting does not solve this problem. What's a workaround here?

snowystinger commented 2 years ago

If I recall our discussion correctly, it centered around the actual implementation of this. Would we interleave the new results such that the Table stayed sorted? If we do that, then how do we inform users what new data has been added and where? How do we inform a screen reader user?

If we just append all the new data to the end, then the sort no longer makes any sense, do we drop the sorting, and how do we communicate that to users? The new data will also have nothing to do with the current sort, so the user might expect to see the next page of sorted data but instead get a non-meaningful order.

The best thing to do is to sort on the server, and when the sort direction or column is changed on the client, the whole dataset should be replaced with the first page of the server sorted data, that way as you load more, you load it in order. Can you explain more of what you mean by "Server-side sorting does not solve this problem"?

flayner2 commented 2 years ago

The best thing to do is to sort on the server, and when the sort direction or column is changed on the client, the whole dataset should be replaced with the first page of the server sorted data, that way as you load more, you load it in order. Can you explain more of what you mean by "Server-side sorting does not solve this problem"?

But how does that translate to, for example, a scroll-based pagination? In that case, if the user changes the order and the list has to be reloaded with the sorted data from the initial request, then the page is gonna be completely reset and the user must load the "extra results" again. E.g., if I'm paginating by each 20 results, in the initial request the user sees the first 20, and then they load 20 more. Now they have 40 results. If they change the sorting order, now they're gonna have only 20 results again, possibly ones that weren't even there to begin with. It doesn't prevent it from being an UX issue, because the user shouldn't expect their results to be cleared and start the browsing all over again.

My half-arsed solution, which isn't clean nor great, is to save the results from each load and loadMore call to a second list, in this case a regular Array, and use that list to display the results and do the sorting. In that sense, the AsyncList pretty much becomes an interface for the requests and loading status shenanigans.

The new data will also have nothing to do with the current sort, so the user might expect to see the next page of sorted data but instead get a non-meaningful order.

The same would happen if you have to resend the initial request with the new sort parameters.

If I recall our discussion correctly, it centered around the actual implementation of this. Would we interleave the new results such that the Table stayed sorted? If we do that, then how do we inform users what new data has been added and where? How do we inform a screen reader user?

If we just append all the new data to the end, then the sort no longer makes any sense, do we drop the sorting, and how do we communicate that to users?

I have absolutely no clue :smiling_face_with_tear: .

snowystinger commented 2 years ago

The new data will also have nothing to do with the current sort, so the user might expect to see the next page of sorted data but instead get a non-meaningful order.

The same would happen if you have to resend the initial request with the new sort parameters.

I disagree, if you dumped all current data on a new sort and loaded the first page of sorted data, then the user would be moved to the top and the data they'd be presented with would be the most important data according to that sort. There could be some fanciness with caching items in order to make subsequent loadMore's faster, but you wouldn't want to keep the current dataset in its entirety because of the interleaving issue.

devongovett commented 2 years ago

The problem is if you have paginated data, and you re-sort on the client side without reloading any more data, you will likely be missing items. For example, if you have a list of numbers sorted ascending from 1 to 1,000, but only 1 to 50 are loaded, when you change the sort order to be descending you will only have 50 to 1 rather than 1,000 to 1.

flayner2 commented 2 years ago

@snowystinger and @devongovett , yes, that certainly is an issue with client-side sorting the data if you're thinking about the most relevant data for that sorting order. But if the user's intent is to sort the currently loaded data, then server-side sorting is not a solution.

devongovett commented 2 years ago

Ok, so say you only sort the loaded data. Now you have 50 to 1. When the user scrolls down and loads more data, what should happen? Would you see 50 to 1 followed by 75 to 50? Would you prepend the data to the top of the list so you have 75 to 1 and shift everything down so the user has to scroll back up to see them? I don't see how either of these are a good user experience. Seems like if you want to sort locally, you shouldn't have pagination.

flayner2 commented 2 years ago

Ok, so say you only sort the loaded data. Now you have 50 to 1. When the user scrolls down and loads more data, what should happen? Would you see 50 to 1 followed by 75 to 50? Would you prepend the data to the top of the list so you have 75 to 1 and shift everything down so the user has to scroll back up to see them? I don't see how either of these are a good user experience. Seems like if you want to sort locally, you shouldn't have pagination.

As I said in my reply, I have no clue about what should actually be done, I'm just pointing out that the current status of not being able to perform a loadMore call after a sort call is also not a solution. It should probably be the dev's responsibility to decide what happens when the new data is loaded after a sort change, and the current status of the API doesn't allow that. But yes, I definitely understand the underlying issue, and I agree when you say that if the expected behavior of the change in sorting order is to load the most relevant results for that variable, and not to change the order of the currently loaded results, than yes, the only thinkable solution is to send a new initial request with a sort parameter.

At this point, it sounds like it mostly comes down to application design, but the current status of the API doesn't really allow the dev to decide what to do if they don't wanna go a side route as I described (by creating a secondary list and etc.).

devongovett commented 2 years ago

As I said in my reply, I have no clue about what should actually be done

Seems like this is the actual issue. useAsyncList needs to know what it should do, and I think it currently chooses the best/most consistent option UX wise. loadMore always appends to the end of the list, and when sorting the cursor is reset so that the sort order is consistent. I guess we could add more options if someone is able to fully define how they should work, but also, we don't force you to use useAsyncList, so if you need to build something custom you can always do so.

dannify commented 2 weeks ago

Without some real direction here, it is unlikely that the team will make any changes to the current API. Since useAsyncList is a convenience hook and not a requirement to use, a custom solution can created for your particular use case. If there is more direction, I am happy to have the issue reopened. Thanks for the discussion.