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

Set sort column and direction with props #649

Merged
merged 7 commits into from
Mar 19, 2018

Conversation

tdzienniak
Copy link
Contributor

Description

A few sentences describing the overall goals of the pull request's commits.

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x")

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)

#547

What is the new behavior?

Sorting direction can be set with props during grid initialization.

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[x] No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 35f6085 on tdzienniak:master into ** on adazzle:master**.

@@ -66,6 +66,8 @@ const ReactDataGrid = React.createClass({
onCellsDragged: React.PropTypes.func,
onAddFilter: React.PropTypes.func,
onGridSort: React.PropTypes.func,
sortColumn: React.PropTypes.string,
sortDirection: React.PropTypes.oneOf(['ASC', 'DESC', 'NONE']),
Copy link
Contributor

Choose a reason for hiding this comment

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

hey @tdzienniak -- this looks good, but one suggestion: can we use the DEFINE_SORT const from here

https://github.com/adazzle/react-data-grid/blob/master/packages/react-data-grid/src/cells/headerCells/SortableHeaderCell.js#L3

rather than having multiple arrays of possible values?

There's probably also an argument to be made for moving the existing array out of SortableHeaderCell.

My two cents are that sortColumn and sortDirection are in the right place here, but the existing definitions should be moved here too? Sorting should conceptually apply to the whole grid, not just HeaderCells..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. Maybe we can move these definitions to Constants.js or AppConstants.js file?

Choose a reason for hiding this comment

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

Hi, i am facing one issue if i want to keep " link" inside the column may i know the process in react data grid.waiting your replay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jpdriver now prop type check is using DEFINE_SORT constants from SortableHeaderCell.js file. I think that moving these definition to other file needs separate PR, maybe global refactor of dealing with constant values as these.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 35f6085 on tdzienniak:master into ** on adazzle:master**.

@stevewillard
Copy link

What's the status of this PR? I would really like this feature.

@jasonpatt
Copy link

Any update on the status of this pull request? This is a needed feature that my team is waiting for.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 86ba455 on tdzienniak:master into ** on adazzle:master**.

@GlebDolzhikov
Copy link

Please add this feature to package, this three lines of code, but very important to have it!
I can't install forked version because of multi packages structure of repo(

@tdzienniak
Copy link
Contributor Author

@jpdriver I can correct any issues with this PR. If there are none, why not merge it? I see that people are interested in this change.

@mgscreativa
Copy link

mgscreativa commented Jul 19, 2017

Hi! In my case I'm using meteor to fetch a paginated list. Triggering onGridSort handler calls a subscription that sorts the data server side then publishes the paginated data back to the client. To get that subscription data I must use createContainer HOC.

In my scenario, it will be just cool to have those props to tweak them in my render function, so after subscription is ready, the render function should get the sort order and column and set that on the ReactDataGrid component because the data got sorted on the server.

Please merge this!

Thanks!

@hiredgunhouse
Copy link

What is the status of this feature?

@mohitgupta8888
Copy link

@malonecj @jpdriver Please add this feature to provide default sortColumn and sortDirection. Thanks.

@mrgurdeep
Copy link
Contributor

@jpdriver @tdzienniak Should you have a suggestion on what change you are expecting. I can make those changes in this PR.

Otherwise we can accept this PR.

Copy link
Contributor

@mrgurdeep mrgurdeep left a comment

Choose a reason for hiding this comment

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

added consts

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 2db8015 on tdzienniak:master into ** on adazzle:master**.

@mlmassey
Copy link

mlmassey commented Dec 2, 2017

Please pull this in. Minor change and is a needed feature for my usage

@jimmed
Copy link

jimmed commented Jan 24, 2018

For those looking to work around this issue without forking the project, you can use React's ref API along with ReactDataGrid's handleSort method, like so:

class MyAwesomeGridWrapper extends Component {

  // ...

  componentDidMount() {
	const { sortColumn, sortDirection } = this.props;
    if (!this.grid || !sortColumn || !sortDirection) return;
	this.grid.handleSort(sortColumn, sortDirection);
  }

  // ...

  render() {
    return (<DataGrid ref={grid => (this.grid = grid)} ... />)
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.