Skip to content
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
sqs merged 5 commits into
mainfrom
sqs/gql-conn
Jul 15, 2024
Merged

make pagination hooks store filter & query params in URL, not just pagination params#63744
sqs merged 5 commits into
mainfrom
sqs/gql-conn

Conversation

@sqs

@sqs sqs commented Jul 10, 2024

Copy link
Copy Markdown
Member

This is a refactor with a few incidental user-facing changes (eg not showing a sometimes-incorrect total count in the UI).


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.

Previously, you could use the options: { useURL: true} arg to usePageSwitcherPagination and useShowMorePaginationUrl. 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:

const connectionState = useUrlSearchParamsForConnectionState(filters)
const { ... } = usePageSwitcherPagination({ query: ..., state: connectionState}) // or useShowMorePaginationUrl

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:

  • remove <ConnectionSummary first> that was used in an erroneous calculation. 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.
  • remove ?visible=N param 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 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.
  • 2 other misc. ones that do not affect user-facing behavior (see commit messages)

Test plan

Visit the site admin repositories and packages pages. Ensure all filter options work and pagination works.

@cla-bot cla-bot Bot added the cla-signed label Jul 10, 2024
@sqs sqs force-pushed the sqs/gql-conn branch 13 times, most recently from 48638f2 to c8cef25 Compare July 14, 2024 21:00
@sqs sqs marked this pull request as ready for review July 14, 2024 21:46
@sqs sqs requested a review from a team July 14, 2024 21:54
@sqs sqs force-pushed the sqs/gql-conn branch 2 times, most recently from dec676a to 3c94fac Compare July 14, 2024 23:53
@sqs sqs requested a review from a team July 15, 2024 02:49
Comment thread client/web/src/components/FilteredConnection/hooks/connectionState.ts Outdated
Comment thread client/web/src/components/FilteredConnection/utils.ts Outdated
@sqs sqs enabled auto-merge (squash) July 15, 2024 18:59
sqs added 5 commits July 15, 2024 12:10
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.
@sqs sqs merged commit e73efbe into main Jul 15, 2024
@sqs sqs deleted the sqs/gql-conn branch July 15, 2024 19:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants