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

Fix for #873 #877

Merged
merged 1 commit into from
Sep 12, 2019
Merged

Fix for #873 #877

merged 1 commit into from
Sep 12, 2019

Conversation

patorjk
Copy link
Collaborator

@patorjk patorjk commented Aug 28, 2019

#873

Quick view: https://codesandbox.io/s/github/patorjk/mui-datatables/tree/sortFail

For test, change customizeFilter example to:

import { FormGroup, FormLabel, TextField } from '@material-ui/core';
import React from 'react';
import ReactDOM from 'react-dom';
import MUIDataTable from '../../src';

class Example extends React.Component {
  render() {
    const data = [
      {
        foo: 1,
        bar: 2
      },
      {
        foo: 3
      }
    ];

    const columns = [
      {
        name: "foo"
      },
      {
        name: "bar"
      }
    ];

    var options = {
      responsive: "scroll"
    };

    return (
      <MUIDataTable title={'ACME Employee list - customizeFilter'} data={data} columns={columns} options={options} />
    );
  }
}

ReactDOM.render(<Example />, document.getElementById('app-root'));

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 75.297% when pulling 73d6889 on patorjk:sortFail into d1914e4 on gregnb:master.

@patorjk
Copy link
Collaborator Author

patorjk commented Aug 28, 2019

Also, when I updated the sortCompare function, I changed it so it wouldn't have any side effects (previously it was modifying a.data and b.data if they were null). The function is only used in 1 place in the code:

https://github.com/gregnb/mui-datatables/blob/master/src/MUIDataTable.js#L1133

The returned sortedData array only gets used for positional information, so the previous side effect didn't have any affect, so it should be safe to remove.

@gabrielliwerant gabrielliwerant added the needs review Useful to mark PRs for what's up next to review label Aug 28, 2019
Copy link
Collaborator

@gabrielliwerant gabrielliwerant left a comment

Choose a reason for hiding this comment

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

Nice catch! Looks good to go. I also added a test #892 to hopefully prevent regressions here in the future.

@gabrielliwerant gabrielliwerant added bug lgtm and removed needs review Useful to mark PRs for what's up next to review labels Sep 3, 2019
@gabrielliwerant gabrielliwerant merged commit 7e1605c into gregnb:master Sep 12, 2019
lalong13 pushed a commit to lalong13/mui-datatables that referenced this pull request Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants