This repository was archived by the owner on Sep 30, 2024. It is now read-only.
make pagination hooks store filter & query params in URL, not just pagination params#63744
Merged
Conversation
48638f2 to
c8cef25
Compare
dec676a to
3c94fac
Compare
camdencheek
approved these changes
Jul 15, 2024
This makes our URLs with a RepositoryCursor in them shorter.
In TestConnectionResolverStoreSuite, the test failure messages were incorrect, referring to different expected values than were actually compared against. Also, it did not support testing with an order-by. Both are fixed.
This was intended to make it so that if you reloaded or navigated directly to a page that had a list, subsequently fetching the next page would only get `pageSize` additional records (eg 20), not `first` (which is however many records were already showing) for batch-based navigation. This is not worth the additional complexity that `?visible=` introduces, and is not clearly desirable even.
…ulation It was only used as a hack to determine the `totalCount` of the connection. This is usually returned in the connection result itself and that is the only value that should be used.
…gination params Make `usePageSwitcherPagination` and `useShowMorePaginationUrl` support storing filter and query params, not just pagination params, in the URL. This is commonly desired behavior, and there are many ways we do it across the codebase. This attempts to standardize how it's done. It does not update all places this is done to standardize them yet.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a refactor with a few incidental user-facing changes (eg not showing a sometimes-incorrect total count in the UI).
Make
usePageSwitcherPaginationanduseShowMorePaginationUrlsupport storing filter and query params, not just pagination params, in the URL. This is commonly desired behavior, and there are many ways we do it across the codebase. This attempts to standardize how it's done. It does not update all places this is done to standardize them yet.Previously, you could use the
options: { useURL: true}arg tousePageSwitcherPaginationanduseShowMorePaginationUrl. This was not good because it only updated the pagination URL querystring params and not the filter params. Some places had a manual way to update the filter params, but it was incorrect (reloading the page would not get you back to the same view state) and had a lot of duplicated code. There was actually no way to have everything (filters and pagination params) updated in the URL all together, except using the deprecated<FilteredConnection>.Now, callers that want the URL to be updated with the connection state (including pagination and filters) do:
Callers that do not want the connection state to be reflected in the URL can just not pass any
state:value.This PR also has some other refactors:
<ConnectionSummary first>that was used in an erroneous calculation. It was only used as a hack to determine thetotalCountof the connection. This is usually returned in the connection result itself and that is the only value that should be used.?visible=Nparam from some connection pages, just use?first=N. This was intended to make it so that if you reloaded or navigated directly to a page that had a list, subsequently fetching the next page would only getpageSizeadditional records (eg 20), notfirst(which is however many records were already showing) for batch-based navigation. This is not worth the additional complexity that?visible=introduces, and is not clearly desirable even.Test plan
Visit the site admin repositories and packages pages. Ensure all filter options work and pagination works.