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.

Sign up for GitHub

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

Jump to bottom

[JSX] Add Loading Bar for Progressive Loading UI #9401

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[JSX] Add Loading Bar for Progressive Loading UI #9401

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9167590
added loadingbar and incorporated into the filterabledatatable
ridz1208 Oct 10, 2024
eb2267f
Update jsx/LoadingBar.tsx
HenriRabalais Oct 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions jsx/FilterableDataTable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import PropTypes from 'prop-types';
import Panel from 'jsx/Panel';
import DataTable from 'jsx/DataTable';
import Filter from 'jsx/Filter';
import LoadingBar from 'jsx/LoadingBar';

/**
* FilterableDataTable component.
Expand Down Expand Up @@ -35,7 +36,6 @@ class FilterableDataTable extends Component {

/**
* Updates filter state
*
Copy link
Contributor

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.

Copy link
Collaborator Author

@HenriRabalais HenriRabalais Oct 25, 2024 •
edited
Loading

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.

Copy link
Contributor

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 !

* @param {object} filters
*/
updateFilters(filters) {
Expand All @@ -48,7 +48,6 @@ class FilterableDataTable extends Component {

/**
* Updates URL Query Params
*
* @param {object} filters
*/
updateQueryParams(filters) {
Expand All @@ -66,7 +65,6 @@ class FilterableDataTable extends Component {

/**
* Add new filter to the filter object
*
* @param {string} name
* @param {*} value
* @param {boolean} exactMatch
Expand All @@ -79,7 +77,6 @@ class FilterableDataTable extends Component {

/**
* Remove filter from the filter object
*
* @param {string} name
*/
removeFilter(name) {
Expand All @@ -99,7 +96,6 @@ class FilterableDataTable extends Component {
* Returns the filter state, with filters that are
* set to an invalid option removed from the returned
* filters
*
* @return {object}
*/
validFilters() {
Expand Down Expand Up @@ -129,7 +125,6 @@ class FilterableDataTable extends Component {

/**
* Renders the React component.
*
* @return {JSX} - React markup for the component
*/
render() {
Expand All @@ -149,7 +144,10 @@ class FilterableDataTable extends Component {
/>
);

const dataTable = (
const {progress} = this.props;
const dataTable = !isNaN(progress) && progress < 100 ? (
<LoadingBar progress={progress}/>
) : (
<DataTable
data={this.props.data}
fields={this.props.fields}
Expand Down Expand Up @@ -191,6 +189,7 @@ FilterableDataTable.propTypes = {
updateFilterCallback: PropTypes.func,
noDynamicTable: PropTypes.bool,
loading: PropTypes.element,
progress: PropTypes.number,
getMappedCell: PropTypes.func,
folder: PropTypes.element,
nullTableShow: PropTypes.element,
Expand Down
56 changes: 56 additions & 0 deletions jsx/LoadingBar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import React from 'react';

interface LoadingBarProps {
progress: number; // Expect progress to be a number
HenriRabalais marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* LoadingBar is a React component that displays a progress bar to indicate
* loading status.
*
* @param {number} progress - A number representing the loading progress (0 to
* 100).
Comment on lines +13 to +14
Copy link
Contributor

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).

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.

Copy link
Collaborator Author

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?

Copy link
Contributor

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.

*
* @returns {JSX.Element|null} - Returns a Loading Bar React component if
* progress is valid, otherwise null.
*/
const LoadingBar: React.FC<LoadingBarProps> = ({progress}) => {
const wrapperStyle: React.CSSProperties = {
margin: '50px 0',
};

const containerStyles: React.CSSProperties = {
height: '5px',
backgroundColor: '#e0e0de',
borderRadius: '50px',
overflow: 'hidden',
};

const fillerStyles: React.CSSProperties = {
height: '100%',
width: `${progress}%`,
backgroundColor: '#E89A0C',
borderRadius: 'inherit',
textAlign: 'right',
transition: 'width 1s linear',
};

const progressBar = progress >= 0 ? (
Copy link
Contributor

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.

Copy link
Collaborator Author

@HenriRabalais HenriRabalais Oct 25, 2024 •
edited
Loading

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.

Copy link
Contributor

@maximemulder maximemulder Oct 26, 2024 •
edited
Loading

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.

<div
style={wrapperStyle}
role="progressbar"
aria-valuenow={progress}
aria-valuemin={0}
aria-valuemax={100}
>
<h5 className="animate-flicker" aria-live="polite">Loading...</h5>
<div style={containerStyles}>
<div style={fillerStyles} />
</div>
</div>
) : null;

return progressBar;
};

export default LoadingBar;
1 change: 1 addition & 0 deletions webpack.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ const resolve: webpack.ResolveOptions = {
FilterableDataTable: path.resolve(__dirname, './jsx/FilterableDataTable'),
FilterForm: path.resolve(__dirname, './jsx/FilterForm'),
Loader: path.resolve(__dirname, './jsx/Loader'),
LoadingBar: path.resolve(__dirname, './jsx/LoadingBar'),
Modal: path.resolve(__dirname, './jsx/Modal'),
MultiSelectDropdown: path.resolve(__dirname, './jsx/MultiSelectDropdown'),
PaginationLinks: path.resolve(__dirname, './jsx/PaginationLinks'),
Expand Down
Loading