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.

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

RouteReportPage.js added table pagination #1133

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

kriistiina
Copy link

[fix] Browser slowdown after executing a Route report with large number of records;
The 4000 row preview slice limit is removed.

Forum Discussion
Library used: Material-UI table pagination

This is an initial version using the MUI example for table pagination.
Additional features/behavior may be added after discussion and approval.

@tananaev
Copy link
Member

I think we discussed making it optional if there number exceeds some limit.

Can you also provide screenshots of what it looks like in desktop and mobile views.

@kriistiina
Copy link
Author

Yes, with some customization, the pagination component can be shown or hidden on a condition.
With the changes I just added, the initial views of Route report look like this for desktop and mobile.

After execution of Route report with large number of records, the pagination component is shown and looks like this for desktop and mobile.

On a following report execution, the pagination component is shown or hidden, based on the number of records in the new report. Reports with less records than the first pagination step do not contain pagination and look like this for desktop and mobile.

Copy link
Member

@tananaev tananaev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I said exceeding some limit, I didn't really mean one page. I was thinking something like 4k limit that we had there or at least 1-2k.

modern/src/reports/RouteReportPage.js Outdated Show resolved Hide resolved
modern/src/reports/RouteReportPage.js Outdated Show resolved Hide resolved
modern/src/reports/RouteReportPage.js Outdated Show resolved Hide resolved
modern/src/reports/RouteReportPage.js Outdated Show resolved Hide resolved
modern/src/reports/RouteReportPage.js Outdated Show resolved Hide resolved
};

const handleChangeRowsPerPage = (event) => {
setRowsPerPage(+event.target.value);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use proper explicit conversion and only if needed. I would also inline it.

[fix] Pagination styling moved to useStyle;
[fix] rowsPerPageOptions updated elements;
[fix] Removed comments;
[fix] onPageChange inlined;
[fix] rowsPerPage renamed to itemsPerPage;
[fix] onRowsPerPageChange inlined;
@kriistiina kriistiina requested a review from tananaev June 14, 2023 15:44
Comment on lines 49 to 58
pagination: {
position: 'static',
bottom: 0,
'& .MuiTablePagination-spacer': {
display: 'none',
},
'& .MuiTablePagination-toolbar': {
justifyContent: 'center',
},
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this for?

@@ -137,7 +140,7 @@ const RouteReportPage = () => {
</TableRow>
</TableHead>
<TableBody>
{!loading ? items.slice(0, 4000).map((item) => (
{!loading ? items.slice(page * itemsPerPage, page * itemsPerPage + itemsPerPage).map((item) => (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tested that it shows 1000 if less than 1000?

Copy link

@LiGuru LiGuru Jun 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you think, is it tested?

{!loading ? items.slice(page * itemsPerPage, items.length > 1000 ? page * itemsPerPage + itemsPerPage : items.length).map((item, id) => (
😁

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's too complicated and not very readable. Also we don't need to do slice if the array fits.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there,

I wanted to suggest adding virtualization, like react-window, to improve the table's performance. It can optimize rendering, especially for large datasets, by only rendering visible rows. This can enhance the user experience by reducing memory usage and providing smoother scrolling.

Consider exploring the integration of react-window for better efficiency. It's a popular library for efficient virtualization in React applications.

Thanks for considering this suggestion!

Best regards,
Mohammad Raufzahed

[fix] avoid .slice() when the array fits;
@kriistiina
Copy link
Author

@tananaev
Sorry about the delay, I was on a business trip last few days. I fixed the issue regarding the visualization of all items when their count is less than 1000 and made the .slice conditional only if the items count exceeds 1000.

@tananaev
Copy link
Member

What do you think about the comment of using the "react-window" or something similar? The same way we do on the main screen for devices.

[fix] infinite scroll in Table component with always visible ReportFilter and TableHead;
@kriistiina
Copy link
Author

I used react-virtuoso to load all report items in a Table component, based on this example. This way, the behavior stays close to the one before with infinite scroll, but without the browser slowdown.

@@ -66,6 +66,7 @@
"eslint-config-airbnb": "^19.0.4",
"eslint-plugin-import": "^2.27.5",
"eslint-plugin-react": "^7.32.2",
"eslint-plugin-react-hooks": "^4.6.0"
"eslint-plugin-react-hooks": "^4.6.0",
"react-virtuoso": "^4.3.10"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this here in dev dependencies?

@@ -17,6 +17,7 @@ export default makeStyles((theme) => ({
position: 'sticky',
left: 0,
display: 'flex',
height: '100%',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please explain this.

</div>
{!loading && (
<TableVirtuoso
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have react-window. Why are we adding another library that basically does the same thing?

Copy link

@LiGuru LiGuru Jun 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was my decision, react-window is pretty small, but needs more work to fit complicated needs.

https://dev.to/sanamumtaz/react-virtualization-react-window-vs-react-virtuoso-8g

MUI Table provided a solution with virtuoso, which is straight forward approach, I think.

And of course you mentioned that there are options "react-window or something similar".

;)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Obviously I didn't mean that it's ok to just add more libraries that do the same thing. If you want to replace the virtualization library, you have to provide justification why the old one is bad and the new one is better. Then migrate all existing use cases to the new library.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let's clarify.
Kristina should implement rendering with react-window.

Kristina should not use additional libraries without clarification by you.

What about adding filters and sorting?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding any new things we can discuss separately first.

[fix] Removed additional package react-virtuoso;
[fix] Infinite scroll with always visible ReportFilter;
@kriistiina
Copy link
Author

I managed to visualize the Route report by using the FixedSizeGrid component from react-window and removed the additional react-virtuoso.

Comment on lines +144 to +148
columnCount={columns.length + 2}
columnWidth={(columns.length + 2) * (width * (phone ? 0.26 : desktop ? 0.1 : 0.18)) >= width ?
width * (phone ? 0.26 : desktop ? 0.1 : 0.18) : width / (columns.length + 2)}
rowCount={items.length > 0 ? items.length : 5}
rowHeight={52}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not look right at all. Why are there a million hardcoded numbers and some crazy calculations?

{({ columnIndex, rowIndex, style }) => {
const item = items[rowIndex];
const columnKey = columns[columnIndex - 2];
return (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The content also doesn't look good. Very hard to understand what's going on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants