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

Make pagination UI-driven, but based upon server's tokens #5185

Merged
merged 6 commits into from
Aug 10, 2022

Conversation

antgamdia
Copy link
Contributor

Description of the change

  • The pagination wasn't working as designed, that is, on the very first load we were retrieving the whole list of packages regardless of the scrolling.
    • Proposed solution: make the UI decide when to paginate, but leave the nextPageToken selection to the backend.

Example of my local's vs what is currently on main just accessing /#/c/default/ns/default/catalog:
image

Benefits

Fewer initial requests and better UX, as endless pagination will work as it is expected to.

Possible drawbacks

N/A

Applicable issues

Additional information

Squeezing a minor linter-related change.

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@netlify
Copy link

netlify bot commented Aug 9, 2022

Deploy Preview for kubeapps-dev failed.

Built without sensitive environment variables

Name Link
🔨 Latest commit 3c69ac5
🔍 Latest deploy log https://app.netlify.com/sites/kubeapps-dev/deploys/62f3d6f8e3ff1000091da67d

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
@@ -149,30 +151,32 @@ export default function Catalog() {
cluster,
namespace,
reposFilter,
nextPageToken,
localNextPageToken,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why renaming the variable? Why is important the nuance of "local"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Becasue nextPageToken is coming from the global packages state and localNextPageToken is a copy of this value, but in the component's state. This way, we have fine-grained control of the lifecycle of the state.
In fact, this is the crux of the PR: nextPageToken gets updated each time new data arrives (that's why it was failing).
So, technically we are not renaming but storing a local copy of nextPageToken and managing how it should be updated (that is, on each pagination event).

Copy link
Collaborator

@castelblanque castelblanque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Conflicts:
	dashboard/src/components/Config/AppRepoList/AppRepoForm.tsx
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Conflicts:
	dashboard/src/components/Catalog/Catalog.test.tsx
	dashboard/src/components/Catalog/Catalog.tsx
@antgamdia antgamdia merged commit f8c9883 into vmware-tanzu:main Aug 10, 2022
@antgamdia antgamdia deleted the 5165-catalogPaginationFix branch August 10, 2022 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants