-
Notifications
You must be signed in to change notification settings - Fork 173
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
[JSX] Add Loading Bar for Progressive Loading UI #9401
base: main
Are you sure you want to change the base?
Conversation
cb142e6
to
59f1c16
Compare
remove CBIGR override comments added typescripting trying to pass validation passing checks added progress to proptypes
82c44cf
to
9167590
Compare
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.
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 ?
* @param {number} progress - A number representing the loading progress (0 to | ||
* 100). |
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.
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
).
* @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.
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.
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?
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.
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.
transition: 'width 1s linear', | ||
}; | ||
|
||
const progressBar = progress >= 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.
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.
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.
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.
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.
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.
@@ -35,7 +36,6 @@ class FilterableDataTable extends Component { | |||
|
|||
/** | |||
* Updates filter state | |||
* |
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.
Is there a reason you removed all these line breaks in this file ? You seem to have them in your new doc comments.
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.
no, must have been an accident or done absent-mindedly — or it was done by linting? honestly not sure.
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.
Okay, just (un)remove them then !
Co-authored-by: Maxime Mulder <[email protected]>
@maximemulder thanks for the review! let me know your thoughts on my responses and hopefully we can get this in a mergeable state soon :) |
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. |
This PR introduces a Loading Bar component, allowing a progress prop to be passed which will fill the bar as it approaches 100%.
The LoadingBar has been incorporated into the the Filterable Datatable so that a Loading Bar can be displayed while data is being fetched.