[JSX] Add ProgressBar to FilterableDataTable for Progressive Loading UI #9401

HenriRabalais · 2024-10-10T17:46:43Z

This PR incorporates the ProgressBar into the the Filterable Datatable so that a progress can be displayed while data is being fetched.

maximemulder

Haven't tested the code, but here is my review. But first of all, the code is of high quality so congrats !

For the small nits, look at my comments on the code.

For other (and larger) design concerns, here are my questions:

I see that the loading bar is not used yet. Do you have a use case for it ?
Wouldn't ProgressBar be a better name ? (no strong opinion here)
I see that the progress is expected to be between 0 and 100 (so that it can be used directly in CSS), but it could also be between 0 and 1, I guess the best option depends on how the progress bar is used ?

jsx/LoadingBar.tsx

maximemulder · 2024-10-24T16:48:59Z

jsx/LoadingBar.tsx

+ * @param {number} progress - A number representing the loading progress (0 to
+ * 100).


This doc comment suggests that LoadingBar has a parameter of type number, but that is not the case as it takes the props object (which has an attribute progress of type number).

Suggested change

* @param {number} progress - A number representing the loading progress (0 to

* 100).

* @param {LoadingBarProps} props - The props of the component

In general, I think param/return doc comments are often redundant when we have typing enabled (although encouraged / mandatory for our CI, so no problem for this PR !), I'll try to discuss that at the next LORIS meeting.

Agreed on the redundancy of typing in docs — I didn't type 'prop' because that feels opaque and what feels relevant to be documented are the actual attributes of the props object.

How would you suggest I document it?

According to the JSDoc documentation, you should have one line for props and one line for props.progress. This is kinda verbose but I guess that's the standard way to do it. But since props is typed with LoadingBarProps, I guess just documentating props is ok, progress is documented in the interface.

Alternatively, if you're patient enough, I'll push for the removal of the @param / @returns requirement in the JSDoc, I agree it's often redundant with Typescript.

maximemulder · 2024-10-24T16:57:36Z

jsx/LoadingBar.tsx

+    transition: 'width 1s linear',
+  };
+
+  const progressBar = progress >= 0 ? (


Shouldn't this always be true ? I think if progress is outside of the expected range it likely means that the caller is doing something wrong.

Hmm, yeah — if I recall correctly this was probably to offload some of the responsibility of revealing and hiding the component from any potential parent onto the component itself. If you don't think that's a good idea I will remove this line and make sure any parent that is using it is responsible for showing and hiding it. Which, in this case, FilterableDataTable is already doing.

IMO after the check to ensure progress is within the accepted range, log an error if it is not and return null. I was hesitating to throw an Error but this component is probably not critical enough to do so (though no strong opinion here), just having the error logged in the console should be enough.

maximemulder · 2024-10-24T16:59:40Z

jsx/FilterableDataTable.js


  /**
   * Updates filter state
-   *


Is there a reason you removed all these line breaks in this file ? You seem to have them in your new doc comments.

no, must have been an accident or done absent-mindedly — or it was done by linting? honestly not sure.

Okay, just (un)remove them then !

HenriRabalais · 2024-10-25T23:23:47Z

@maximemulder thanks for the review! let me know your thoughts on my responses and hopefully we can get this in a mergeable state soon :)

maximemulder · 2024-10-26T19:13:38Z

Your answers to the nits are fine but please also look a the few questions I wrote in the comment. I don't need changes to answer these if you disagree but I just want to make sure these are considered.

HenriRabalais · 2024-10-28T16:56:26Z

To answer your questions:

Yes, I will be making a PR soon with the biobank module which will make use of it.
There's already a ProgressBar Component... so actually that makes the addition of LoadingBar redundant. I'll modify this PR to make any changes that I feel that I need to the ProgressBar Component itself.
The ProgressBar uses 0-100 so I guess we'll keep it that way.

maximemulder · 2024-10-28T18:37:37Z

Okay ! Just do the following changes and I'll approve the PR:

Fix the JSDoc @param so that it follows the official documentation format (or omit it if your prefer, it is a warning that is already ignored in plenty of other places).
Un-remove the line breaks in FilterableDataTable.js
Add a guard that logs an error in the console and returns null if the value of progress is incorrect.

HenriRabalais · 2024-10-31T13:21:48Z

@maximemulder I reverted to using the already-existing ProgressBar since I want to avoid creating another component that serves the same function. The changes I would have made to the ProgressBar are all aesthetic, so I will keep them for another PR.

maximemulder · 2024-10-31T13:54:48Z

Okay ! I guess I will close this PR then. Looking forward for your next PR (when it is ready).

HenriRabalais · 2024-10-31T16:31:40Z

No need to close it, the PR is ready as is! The important part was to incorporate some sort of progress bar into the filterable datatable.

maximemulder

Whoops, my fault, I did not see the changes and thought that you were only doing another PR. Looks okay !

remove CBIGR override comments added typescripting trying to pass validation passing checks added progress to proptypes Update jsx/LoadingBar.tsx Co-authored-by: Maxime Mulder <maxime-mulder@outlook.com> updated to use ProgressBar instead of LoadingBar removed LoadingBar remove accidental space remove swap file

…UI (aces#9401) This incorporates the ProgressBar into the the Filterable Datatable so that a progress can be displayed while data is being fetched.

HenriRabalais force-pushed the 2024-10-10_loadingbar branch from cb142e6 to 59f1c16 Compare October 10, 2024 17:50

HenriRabalais added the Area: UI PR or issue related to the user interface label Oct 10, 2024

HenriRabalais force-pushed the 2024-10-10_loadingbar branch from 82c44cf to 9167590 Compare October 10, 2024 19:50

HenriRabalais requested a review from maximemulder October 24, 2024 14:04

maximemulder requested changes Oct 24, 2024

View reviewed changes

regisoc assigned jeffersoncasimir Oct 29, 2024

maximemulder unassigned jeffersoncasimir Oct 29, 2024

maximemulder closed this Oct 31, 2024

HenriRabalais reopened this Oct 31, 2024

HenriRabalais changed the title ~~[JSX] Add Loading Bar for Progressive Loading UI~~ [JSX] Add ProgressBar to FilterableDataTable for Progressive Loading UI Oct 31, 2024

maximemulder approved these changes Oct 31, 2024

View reviewed changes

HenriRabalais force-pushed the 2024-10-10_loadingbar branch from 8e56c02 to c0f5c57 Compare October 31, 2024 16:38

maximemulder added the Language: Javascript PR or issue that update Javascript code label Nov 6, 2024

driusan merged commit 8451f3b into aces:main Nov 7, 2024
10 checks passed

		* @param {number} progress - A number representing the loading progress (0 to
		* 100).

	* @param {number} progress - A number representing the loading progress (0 to
	* 100).
	* @param {LoadingBarProps} props - The props of the component

[JSX] Add ProgressBar to FilterableDataTable for Progressive Loading UI #9401

[JSX] Add ProgressBar to FilterableDataTable for Progressive Loading UI #9401

Uh oh!

Conversation

HenriRabalais commented Oct 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maximemulder left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maximemulder Oct 24, 2024

Choose a reason for hiding this comment

Uh oh!

HenriRabalais Oct 25, 2024

Choose a reason for hiding this comment

Uh oh!

maximemulder Oct 26, 2024

Choose a reason for hiding this comment

Uh oh!

maximemulder Oct 24, 2024

Choose a reason for hiding this comment

Uh oh!

HenriRabalais Oct 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maximemulder Oct 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maximemulder Oct 24, 2024

Choose a reason for hiding this comment

Uh oh!

HenriRabalais Oct 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maximemulder Oct 26, 2024

Choose a reason for hiding this comment

Uh oh!

HenriRabalais commented Oct 25, 2024

Uh oh!

maximemulder commented Oct 26, 2024

Uh oh!

HenriRabalais commented Oct 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maximemulder commented Oct 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HenriRabalais commented Oct 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maximemulder commented Oct 31, 2024

Uh oh!

HenriRabalais commented Oct 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maximemulder left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

HenriRabalais commented Oct 10, 2024 •

edited

Loading

maximemulder left a comment •

edited

Loading

HenriRabalais Oct 25, 2024 •

edited

Loading

maximemulder Oct 26, 2024 •

edited

Loading

HenriRabalais Oct 25, 2024 •

edited

Loading

HenriRabalais commented Oct 28, 2024 •

edited

Loading

maximemulder commented Oct 28, 2024 •

edited

Loading

HenriRabalais commented Oct 31, 2024 •

edited

Loading

HenriRabalais commented Oct 31, 2024 •

edited

Loading