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

[Table] Fix Sorting & Selecting tables #36898

Merged
merged 2 commits into from
Apr 22, 2023

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Apr 15, 2023

Revert #33236, the logic is 1. has bugs, 2. duplicate the filtering logic, and 3. do the opposite of what's recommended:

  1. By "bugs", I mean, open https://deploy-preview-33236--material-ui.netlify.app/material-ui/react-table/#customization and see how the scroll position is wrong, the server-side render is missing the rows.

Screenshot 2023-04-15 at 22 36 43

  1. By "duplicated", see the number of times stableSort() is written.
  2. By "opposite", see https://react.dev/learn/you-might-not-need-an-effect#caching-expensive-calculations.

To make the review of the changes easier, I have done two commits 1. revert 2. a fix for the original issue. But in practice, I doubt that this has any kind of impact on the operation. The memo is fast, even with 10,000 records (but how do you fetch 10,000 records client-side?). I was close to simply proposing a revert.

@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation component: table This is the name of the generic UI component, not the React module! regression A bug, but worse labels Apr 15, 2023
@mui-bot
Copy link

mui-bot commented Apr 15, 2023

Netlify deploy preview

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

Bundle size report

No bundle size changes

Generated by 🚫 dangerJS against 5bc68f1

);
const handleRequestSort = (event, property) => {
const isAsc = orderBy === property && order === 'asc';
setOrder(isAsc ? 'desc' : 'asc');

Choose a reason for hiding this comment

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

What do you think about simplifying this?

const handleRequestSort = (event, property) => {
  setOrder(orderBy === property && order === 'asc' ? 'desc' : 'asc');
  setOrderBy(property);
};

Copy link
Member Author

Choose a reason for hiding this comment

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

We could but it won't make much a difference. isAsc has an interesting semantic meaning.

@oliviertassinari oliviertassinari merged commit eea15c2 into mui:master Apr 22, 2023
@oliviertassinari oliviertassinari deleted the fix-tables branch April 22, 2023 09:45
@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Apr 22, 2023
@ZeeshanTamboli
Copy link
Member

@oliviertassinari Awesome work! I learned a new thing! :D Adding the useMemo and thus removing the inefficient useEffect changed the game. Thanks. However, I can't find the bug you mentioned in point 1.

The memo is fast, even with 10,000 records

Where did you get this number from?

@oliviertassinari
Copy link
Member Author

I can't find the bug you mentioned in point 1.

@ZeeshanTamboli The scrollbar is messed-up. Disable JavaScript, you will see how the list is empty:

Screenshot 2023-04-24 at 13 08 02

Where did you get this number from?

It's really fast

let array = [...new Array(10000)].map((x, index) => index); 
console.time('filter');
array.filter((x) => `${x}` === '1000');
console.timeEnd('filter');

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: table This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation regression A bug, but worse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants