Support for dynamic pagination #3386
Conversation
Or not ... they've already pushed a fixed 21.1.0-1 even though the PR hasn't been merged, and the old dependency seems to be working again. 🍤 |
There was a problem hiding this comment.
I have a lot of comments. Just work your way through them. Many of them are repeated call outs of the same issues in different code.
My biggest concern is your overhaul of the date picker component (which you don't mention in the description for this PR). I'm not sure what prompted it, and I'm concerned that it's not something that we want.
MVarshini
force-pushed
the
PBENCH-732
branch
2 times, most recently
from
580d931
to
809145b
Compare
The Date Range filter is not working in the staging server and not able to find the root cause as it works well in the local. So, tried to revamp it. |
There was a problem hiding this comment.
This update resolved a number of my comments -- thanks! However, a number of notable remain:
- Problem in the sparse array probing code.
- Sizing and resizing of the results array.
- Needless
Dateobject instantiations - Needless
Objectinstantiation - Needless maintenance of
totalDatasetswhen we have thepublicDataarray - Dual representations of the current page
- Problematic uses of the chaining reference operator (1, 2)
And, the rework of the DatePicker with respect to localtime vs. UTC, which I hope we can discuss in real-time tomorrow.
(And, there are a few open questions and smaller items.)
And, there are the two items, below. 🙂
There was a problem hiding this comment.
We're almost there!
- I think we still need to address the problem of a size mismatch between the results array and the total number of results (e.g., if a result is deleted or added between when a user loads one page and tries to load another). Currently, the code allocates a new array of the appropriate size if the current array is zero-length; I think it should be allocating a new array any time the current array's length is different from the total number of results. This is because, if any results are added or removed, then the indices of the existing results might change, so their positions in the current array are likely to be incorrect, which will lead to problems displaying them (individual results will be lost, omitted, or displayed more than once). So, if we can detect a change in the results list, we should just dump our copy and reload it.
- There's an off-by-one error in the data fetching which may result in refetching a page which we already have.
- And there's some confusion (perhaps all on my part, for which I apologize) which it would be good to alleviate around the
SET_PAGE_OFFSETRedux action: it should be renamed and the expression for its initial value should be changed. - There's an issue with the setting of the "next results index" when we're on the last page. I think it should probably be set to "last plus one". (I don't think it should be left pointing to its previous value.)
Beyond those, there are a couple of lingering nits (1, 2), but I'm not inclined to block the merge over them.
Pagination for Browsing Page favorites pagination add constants
There was a problem hiding this comment.
OK, we're almost there.
- There's still the question around what to do with the results offset when we're at the last page.
- I think that
TYPES.SET_PAGE_OFFSETis mis-named. (And the constant for its start value could use a better name, as well.) - There's a small wrinkle in the code for the reallocation of the results array.
Beyond those, I've some smaller things, like it seems like we're constructing new Date objects more than we need to.
PBENCH-732
Support /datasets/list pagination to improve scalability and robustness