[docs] Update the order of operations for pagination example so that slicing takes place after sorting.

[marceliwac]

This is a very minor tweak to the comment for non-IE11-supported pagination. The comment suggests using .sort instead of stableSort, but contrary to to example code which is valid, the comment's order of operations is wrong, resulting in a list of elements that are sorted, but only within the already-sliced array. Simply put, the suggestion in the comment introduces a bug.

Signed-off-by: Marceli Wac [email protected]

• I have followed (at least) the PR section of the contributing guide.

[mui-bot]

	Messages
📖	Netlify deploy preview: https://deploy-preview-34189--material-ui.netlify.app/

No bundle size changes

Generated by 🚫 dangerJS against a19794e

[michaldudak]

Good catch! Could you please run yarn docs:typescript:formatted to generate the JS version of the demo and commit the changes?

[marceliwac]

@michaldudak Thanks, should be good now!

[michaldudak]

One more thing to do. It seems your branch was based on master when we had problems with regression tests. Could you merge in the current master to fix the regression tests issue? It should be good to merge afterwards.

[marceliwac]

@michaldudak Seems like this has gone through now :)

[michaldudak]

All right! Thanks for your work!

[frontendlane]

I believe this PR was an error and that the original comment was correct.

.sort() sorts elements in-place so the best practice is to first call .slice() in order to create a copy of the original array and only then .sort().

[marceliwac]

Hi @frontendlane,
If you were to slice an array first and sort it afterwards, you would only sort within the slice. While I see your point regarding sorting in place, I do believe that's a desired behaviour. Other option (if for some reason you really don't want to sort the array in place) would be to duplicate the array prior to sorting using something like array destructuring etc:

[...array]
   .sort()
   .slice()

But the slicing operation still needs to happen after sorting to account for all elements in the array.

[frontendlane]

Thanks for replying @marceliwac.

I still think the comment is incorrect. I believe there's a misunderstanding and I should have been clearer in my original comment.

The comment mentions what to use in order to replace the call to stableSort(), not the entire chain stableSort(<code>).sort(<code>).map(<code>).

stableSort() first duplicates and then stable sorts the duplicate array.

Modern browsers have changed their implementation of Array.prototype.sort() such that they perform a stable sort. What didn't change is that Array.prototype.sort() still sorts in-place, so to replicate the functionality of stableSort() in non-IE (i.e. modern) browsers you need to do first duplicate the array and then use the native Array.prototype.sort() like so .sort(getComparator(order, orderBy)) in order to do the actual sorting.

The .slice(page * rowsPerPage, page * rowsPerPage + rowsPerPage) part that comes after stableSort() should remain where it is, because, like you said, you need only a subset of the rows because of the pagination. So, there would be two calls to .sort().

To clarify, if you don't have to support IE your code lines 339 - 341 would look like this:

{rows.slice().sort(getComparator(order, orderBy))
                .slice(page * rowsPerPage, page * rowsPerPage + rowsPerPage)
                .map((row, index) => {

I think the commend should be expanded to explain that modern browsers (i.e. non-IE) browsers render a call to stableSort() redundant because Array.prototype.sort() implements a stable sort.

[marceliwac]

@frontendlane
I see your point now and absolutely agree. What led me to change the above code in the first place was the fact that call to slice() seemed to suggest that it should replace the one implementing pagination, and not an additional call (to copy the array).

You're also welcome to submit a PR with your comments. I will create it in the next few days otherwise. Thanks for bringing this up!

[frontendlane]

Thanks @marceliwac , I'll create a PR tomorrow.