-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
base: master
Are you sure you want to change the base?
Conversation
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. |
Yes, with some customization, the pagination component can be shown or hidden on a condition. 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. |
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.
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.
}; | ||
|
||
const handleChangeRowsPerPage = (event) => { | ||
setRowsPerPage(+event.target.value); |
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.
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;
pagination: { | ||
position: 'static', | ||
bottom: 0, | ||
'& .MuiTablePagination-spacer': { | ||
display: 'none', | ||
}, | ||
'& .MuiTablePagination-toolbar': { | ||
justifyContent: 'center', | ||
}, | ||
}, |
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.
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) => ( |
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.
Have you tested that it shows 1000 if less than 1000?
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.
What you think, is it tested?
{!loading ? items.slice(page * itemsPerPage, items.length > 1000 ? page * itemsPerPage + itemsPerPage : items.length).map((item, id) => (
😁
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 think it's too complicated and not very readable. Also we don't need to do slice
if the array fits.
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.
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;
@tananaev |
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;
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. |
modern/package.json
Outdated
@@ -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" |
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 we need this here in dev dependencies?
@@ -17,6 +17,7 @@ export default makeStyles((theme) => ({ | |||
position: 'sticky', | |||
left: 0, | |||
display: 'flex', | |||
height: '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.
Please explain this.
</div> | ||
{!loading && ( | ||
<TableVirtuoso |
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.
We already have react-window
. Why are we adding another library that basically does the same thing?
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.
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".
;)
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.
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.
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.
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?
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.
Adding any new things we can discuss separately first.
[fix] Removed additional package react-virtuoso; [fix] Infinite scroll with always visible ReportFilter;
I managed to visualize the Route report by using the FixedSizeGrid component from react-window and removed the additional react-virtuoso. |
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} |
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 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 ( |
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.
The content also doesn't look good. Very hard to understand what's going on.
[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.