-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat: Add pagination to data browser #2659
Conversation
I will reformat the title to use the proper commit message syntax. |
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. |
Uffizzi Ephemeral Environment
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great feature!
- There are the vars
const MAX_ROWS = 200;
andconst MAX_ROWS_FETCHED = 200;
which set a hard limit to 200 rows. I think these need to be dynamic now that the number of rows per page can be set manually. For example, if we add a new value like 500 to the list of{[10, 20, 50, 100, 200, 500].map((size)
, there should be 500 rows rendered. I thinkquery.limit(MAX_ROWS_FETCHED);
essentially makes the dashboard fetch 200 rows, but if the rows/page value is lower, less rows get displayed, so the dashboard may be fetching rows that are not even shown? - When clicking on the next / previous button, the data browser should jump to the top. Otherwise, especially when clicking on "next", the user'd have to scroll up to continue seeing data that is sorted by a date for example.
- Instead of adding an empty space to the top of the BrowserFooter, add a light gray line and ensure the bar is the same hight as the bottom bar in the menu column on the left. Also reduce size of next/previous buttons.
- The number of items per page should be set dashboard wide and remembered across sessions. I assume dashboard already has a data store for that. Let's do this: after install, dashboard has a default rows/page of 100. If the user changes the value, that value is stored in the data store and used as default rows/page value for all newly opened dashboard tabs and sessions. In other words, changing the value in any tab simply sets that value as the new default for all newly opened tabs.
- Reset current page when changing class or applying filter. For example, currently, when on page 2 and then navigating to a class that has only 1 page, no objects are displayed because page 2 does not exist.
- Adjust font size
What do you mean by
|
Uffizzi Ephemeral Environment
|
What do you mean? From my testing they only disable when there isn't enough data |
Yes, I have observed the buttons disabled even though there was more data. I was able to reproduce the issue several times. Have there been any changes in the recent commits that could have caused this? I haven't observed that when I wrote the previous feedback. |
Sure, let me try |
@mtrezza let me know how you go when you get the chance 😊 |
Sure, I'll take a look in a bit... |
Uffizzi Ephemeral Environment
|
I was able to reproduce.
I think that is because the total number of records in the class is also determined to be "0", as shown next to the class name. After refreshing the browser tab, the records counter updates to the correct value and paging works. I understand that the paging is depending on that counter value, that's fine. If the number of total records in a class did not update also before this PR, i.e. the bug was not introduced by this PR, then we can ignore it in this PR. And could you please fix the merge conflicts? Screen.Recording.2025-03-23.at.15.56.24.mov |
@dblythy could you please fix the lint issues, and when this is ready, could you please request a review? |
Uffizzi Ephemeral Environment
|
Uffizzi Ephemeral Environment
|
# [7.1.0-alpha.1](7.0.1-alpha.1...7.1.0-alpha.1) (2025-04-06) ### Features * Add pagination to data browser ([#2659](#2659)) ([a3c8a11](a3c8a11))
🎉 This change has been released in version 7.1.0-alpha.1 |
New Pull Request Checklist
Issue Description
Closes: #2571
Approach
TODOs before merging