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

[docs][Table] Refactor Sorting & Selecting table demo #33236

Merged
merged 20 commits into from
Mar 24, 2023

Conversation

IFaniry
Copy link
Contributor

@IFaniry IFaniry commented Jun 20, 2022

When you toggle the "Dense padding" switch, the heavy client-side sorting and pagination logics are unnecessarily triggered.

  • I have moved the sorting and pagination logics into their respective handlers.
  • I have renamed some variables for readability.
  • I have created some constants for convenience.

The demo is making React render function too computationally expensive.
@mui-bot
Copy link

mui-bot commented Jun 20, 2022

Netlify deploy preview

https://deploy-preview-33236--material-ui.netlify.app/

Bundle size report

No bundle size changes

Generated by 🚫 dangerJS against 4d67993

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 28, 2022
@zannager zannager added the component: pagination This is the name of the generic UI component, not the React module! label Oct 25, 2022
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 8, 2023
@ZeeshanTamboli ZeeshanTamboli added the docs Improvements or additions to the documentation label Mar 8, 2023
@ZeeshanTamboli ZeeshanTamboli changed the title Refactor Demo Table sorting and pagination handlers [docs] Refactor Demo Table sorting and pagination handlers Mar 8, 2023
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

Changing the number of rows per page (Rows per page) isn't working.

Preview: https://deploy-preview-33236--material-ui.netlify.app/material-ui/react-table/#sorting-amp-selecting

@ZeeshanTamboli ZeeshanTamboli changed the title [docs] Refactor Demo Table sorting and pagination handlers [docs] Refactor sorting and pagination handlers in Sorting and Selecting table demo Mar 8, 2023
@ZeeshanTamboli ZeeshanTamboli added component: table This is the name of the generic UI component, not the React module! and removed component: pagination This is the name of the generic UI component, not the React module! labels Mar 8, 2023
@ZeeshanTamboli ZeeshanTamboli changed the title [docs] Refactor sorting and pagination handlers in Sorting and Selecting table demo [docs][Table] Refactor sorting and pagination handlers in Sorting and Selecting table demo Mar 8, 2023
@IFaniry
Copy link
Contributor Author

IFaniry commented Mar 15, 2023

Changing the number of rows per page (Rows per page) isn't working.

Preview: https://deploy-preview-33236--material-ui.netlify.app/material-ui/react-table/#sorting-amp-selecting

@ZeeshanTamboli Apologies for the many unsuccessful commits that may have bothered you with notifications.

Now, the preview link you gave me works!

Thank you.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 17, 2023
Signed-off-by: RAMANOHISOA Tahina Faniry Willy <faniry.consulting@gmail.com>
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 20, 2023
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

@IFaniry I found one more thing while testing:

When you go to the last page, which has some empty rows, and then you change the number of rows per page, it still shows the empty rows and does not occupy them. While it does not happen in the latest deployed version: https://mui.com/material-ui/react-table/#sorting-amp-selecting.

Also please update with the latest master branch to include #36546.

docs/data/material/components/table/EnhancedTable.tsx Outdated Show resolved Hide resolved
Co-authored-by: Zeeshan Tamboli <zeeshan.tamboli@gmail.com>
Signed-off-by: RAMANOHISOA Tahina Faniry Willy <faniry.consulting@gmail.com>
@IFaniry
Copy link
Contributor Author

IFaniry commented Mar 22, 2023

@IFaniry I found one more thing while testing:

When you go to the last page, which has some empty rows, and then you change the number of rows per page, it still shows the empty rows and does not occupy them. While it does not happen in the latest deployed version: https://mui.com/material-ui/react-table/#sorting-amp-selecting.

When you change the number of rows per page then you automatically go to the first page and afterwards the last page still works as expected.

So, when you test the deploy link now, it works as expected, right?

@ZeeshanTamboli
Copy link
Member

@IFaniry I found one more thing while testing:
When you go to the last page, which has some empty rows, and then you change the number of rows per page, it still shows the empty rows and does not occupy them. While it does not happen in the latest deployed version: https://mui.com/material-ui/react-table/#sorting-amp-selecting.

When you change the number of rows per page then you automatically go to the first page and afterwards the last page still works as expected.

So, when you test the deploy link now, it works as expected, right?

@IFaniry What I meant was this:

In https://deploy-preview-33236--material-ui.netlify.app/material-ui/react-table/#sorting-amp-selecting, see the empty space on last page after changing rows per page:

issue.sorting.and.pagination.demo.mov

While the working version in https://mui.com/material-ui/react-table/#sorting-amp-selecting (no empty space):

Working.version.mov

@IFaniry
Copy link
Contributor Author

IFaniry commented Mar 22, 2023

Good catch @ZeeshanTamboli ! I made a commit to fix my bug.

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

One more thing I found is that when you change the rows per page or go to the next page, the rows get disorganized. It should be kept sorted in ascending order.

@ZeeshanTamboli ZeeshanTamboli changed the title [docs][Table] Refactor sorting and pagination handlers in Sorting and Selecting table demo [docs][Table] Refactor Sorting & Selecting table demo Mar 24, 2023
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

@IFaniry It looks good and it's a great first pull request on MUI 👌 Thank you for working on it!

@ZeeshanTamboli ZeeshanTamboli merged commit 952d82a into mui:master Mar 24, 2023
@IFaniry
Copy link
Contributor Author

IFaniry commented Mar 24, 2023

yeahhh 🤩 Thank you for supporting me on this PR, @ZeeshanTamboli. You are a great maintainer 👊

oliviertassinari added a commit to oliviertassinari/material-ui that referenced this pull request Apr 15, 2023
@oliviertassinari
Copy link
Member

Reverted in #36898

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: table This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants