-
Notifications
You must be signed in to change notification settings - Fork 16
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
149 - Configure manual pagination in the Files Table UI #156
149 - Configure manual pagination in the Files Table UI #156
Conversation
…b.com/IQSS/dataverse-frontend into feature/149-react-table-manual-pagination-setup
…ps://github.com/IQSS/dataverse-frontend into feature/149-react-table-manual-pagination-setup
I still have to review the code, but I have already tested Storybook and I have found this unexpected behavior: In file selection. The zip limit is not shown when selecting two files from different pages (their sum exceeds the limit): |
constructor( | ||
public readonly page: number = 1, | ||
public readonly pageSize: number = 10, | ||
public readonly total: number = 0 |
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.
By reading this model without much context I don't understand what this "total" property refers to and why it can be 0. Maybe renaming it can help?
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.
I see 🤔 This total refers to the total number of files, the total number of elements to be paginated, maybe rename it to totalFiles? filesTotal?
The total number of pages is called here totalPages so I guess we can call this totalFiles
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.
totalFiles makes sense!
const rowSelection: Record<string, boolean> = {} | ||
|
||
for (let i = 0; i < numberOfRows; i++) { | ||
rowSelection[i as unknown as string] = true |
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.
Why this kind of double casting here? just curious
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.
Typescript doesn't allow conversion of type number to string, so the unknown is an intermediate step to convert the number to string. I'm forcing the type conversion because I think it's a controlled situation, but it would be best if I do the proper casting String(i). Yep, I can change this for better readability and proper casting
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.
Code looks good! Just a couple of comments. Also, please take a look at my comment about the Storybook test.
I have created this issue: IQSS/dataverse-client-javascript#86 We need a use case to get the total space consumed by the files of a particular dataset version. This use case will allow the SPA to know the total space consumed by the files when the user selects "select all" in the files tab, without needing to retrieve all the files (without pagination, could be an expensive operation) and apply client side logic to calculate the size. Once the use case is ready, we will integrate it into the UI logic of the files tab. |
@GPortas I added some commits with the requested changes |
@@ -48,8 +49,10 @@ export function ZipDownloadLimitMessage({ selectedFiles }: ZipDownloadLimitMessa | |||
) | |||
} | |||
|
|||
function getFilesTotalSizeInBytes(files: File[]) { | |||
return files.map((file) => file.size).reduce((bytes, size) => bytes + size.toBytes(), 0) | |||
function getFilesTotalSizeInBytes(files: (File | undefined)[]) { |
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.
Why do you consider undefined
now and not before?
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.
because before your review I wasn't counting with the selectAll for manual pagination, which means that I need to select all files which I don't know because the pagination only retrieves 10 files, so those files that I cannot know are saving their place in the fileSelection list with an undefined
then the fileSelection object can have File | undefined, so this function getFilesTotalSizeInBytes
that is reading the fileSelection now needs to accept some undefined files, to get the totalSizeInBytes it just ignores the undefined files
in the future we'll get the totalFileSize using a different endpoint but for the moment it can only read the size of the files of the current page
@MellyGray This works well, thanks. I did find one difference from the current UI which is unintuitive and was an issue we'd encountered before, why we added that select all link: selecting the global checkbox on a page in the existing ui selects all rows in the current page only. In this pr it selects all files in the dataset. |
@kcondon Global checkbox: selects all rows in the current page -> This is wrong in this PR I can fix that, should we move this back to in progress to work on that fix? Or do we leave this in QA while I work on the fix? Sorry, I get lost in the workflow |
@MellyGray Yes, that is correct. This part of the workflow is a bit informal -if you are able to work on it and it is relatively small, just leave it in QA. If it will stay for a while or very large, move back to In Progress. My motto is whatever works. |
@kcondon ok, I committed the fix! It was a small change after all |
…pagination-setup 149 - Configure manual pagination in the Files Table UI
What this PR does / why we need it:
This PR changes the configuration of the Files Table UI to use manual pagination instead of the automatic pagination provided by react-table. We need this change because we decided to apply server-side pagination to the Files Table so the API will retrieve the data by pages instead of retrieving the whole list of files at once.
Which issue(s) this PR closes:
Special notes for your reviewer:
Changes applied:
Suggestions on how to test this:
npm i
cd packages/design-system && npm run build
cd ../../
npm run storybook
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
No
Is there a release notes update needed for this change?:
Applied manual pagination to the Files Table UI
Additional documentation: