group by with object array and string array #857

d4nj0nes · 2017-06-29T12:42:28Z

Description

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

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/adazzle/react-data-grid/blob/master/CONTRIBUTING.md
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

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)
When using grouping you need to override the default toolbar and row renderer to display user friendly names for the columns.

Could only render a custom toolbar by rendering inline in the property declaration rather than via an import

What is the new behavior?
You can pass in the groupBy prop as an object array.
It assumes current behaviour - that groupBy is an array of strings.
If it is not it expects the objects to have a key and name property.
It will use name to render the groupBy button and the row grouping header.

The toolbar property can only be a React Element not a function. When defining a Generic custom toolbar that enables filtering and grouping in a separate file in TypeScript, nothing was rendered as it failed this validation.

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 · 2017-06-29T16:22:36Z

Changes Unknown when pulling c04c671 on dj-grouping-by-column-name into ** on master**.

coveralls · 2017-06-29T17:12:01Z

Changes Unknown when pulling a26432f on dj-grouping-by-column-name into ** on master**.

diogofcunha

Minor stuff only pal, good job 👍

diogofcunha · 2017-06-30T09:36:11Z

packages/react-data-grid-addons/src/data/RowGrouper.js

@@ -21,7 +21,7 @@ class RowGrouper {

  groupRowsByColumn(rows, columnIndex = 0) {
    let nextColumnIndex = columnIndex;
-    let columnName = this.columns[columnIndex];
+    let columnName = this.columns.length > 0 && typeof this.columns[columnIndex] === 'string' ? this.columns[columnIndex] : this.columns[columnIndex].key;


Is the length ever <=0?

yeah - if you render the grid before grouping which I want to - columns here are the grouped columns not all grid columns

diogofcunha · 2017-06-30T09:41:26Z

packages/react-data-grid-addons/src/toolbars/GroupedColumnsPanel.js

@@ -31,7 +31,10 @@ class GroupedColumnsPanel extends Component {

  renderGroupedColumns() {
    return this.props.groupBy.map(c => {
-      return (<GroupedColumnButton key={c} name={c} onColumnGroupDeleted={this.props.onColumnGroupDeleted}/>);


Would rather not repeat the jsx assignments, it can be:

const groupedColumnButtonProps = { columnKey: c, name: c, onColumnGroupDeleted: this.props.onColumnGroupDeleted, key: typeof c === 'string' ? c : c.key } return <GroupedColumnButton {...groupedColumnButtonProps} />;

got it 👍

But the props change - c is either a string or an object. I'll work it out

it's doing that check in the key assignment in my snippet, you can do it for what changes

just needed to do it for the other props too name and key
Done.

diogofcunha · 2017-06-30T09:42:55Z

packages/react-data-grid/src/RowGroup.js

@@ -95,7 +95,7 @@ const  DefaultRowGroupRenderer = (props) => {
  return (
    <div style={style} onKeyDown={onKeyDown} tabIndex={0}>
      <span className="row-expand-icon" style={{float: 'left', marginLeft: marginLeft, cursor: 'pointer'}} onClick={props.onRowExpandClick} >{props.isExpanded ? String.fromCharCode('9660') : String.fromCharCode('9658')}</span>
-      <strong>{props.columnGroupName} : {props.name}</strong>


What does this change in the actual render? can you show?

so it would say
sellerName: ITV
I just want it to say the value of the group not the column name i.e. ITV
We could make that optinal - but to me it just looks messy

My main concern would be breaking this for people that are already using it, because they are expecting the previous and we will change it in our core and break their functionality

sure - then I'll look at making that optional

coveralls · 2017-06-30T12:34:40Z

Changes Unknown when pulling 3249711 on dj-grouping-by-column-name into ** on master**.

diogofcunha · 2017-07-03T09:35:52Z

packages/react-data-grid/src/RowGroup.js

@@ -95,7 +95,7 @@ const  DefaultRowGroupRenderer = (props) => {
  return (
    <div style={style} onKeyDown={onKeyDown} tabIndex={0}>
      <span className="row-expand-icon" style={{float: 'left', marginLeft: marginLeft, cursor: 'pointer'}} onClick={props.onRowExpandClick} >{props.isExpanded ? String.fromCharCode('9660') : String.fromCharCode('9658')}</span>
-      <strong>{props.columnGroupName} : {props.name}</strong>


My main concern would be breaking this for people that are already using it, because they are expecting the previous and we will change it in our core and break their functionality

coveralls · 2017-07-03T11:05:13Z

Changes Unknown when pulling e69e2d4 on dj-grouping-by-column-name into ** on master**.

group by with object array and string array

28eb21a

d4nj0nes requested a review from diogofcunha June 29, 2017 12:42

Daniel Jones added 4 commits June 29, 2017 13:47

styling issues and missing prop

8237f8a

casing in ToolBar definition - fix undef lint error

73afd7b

missing import for isFunction

feea151

fix tests and enable karma debugging

c04c671

ignore vs code and remove fdescribe from test

a26432f

diogofcunha suggested changes Jun 30, 2017

View reviewed changes

don't repeat assignment of props

3249711

diogofcunha suggested changes Jul 3, 2017

View reviewed changes

put column name back

e69e2d4

diogofcunha merged commit 6900b30 into master Jul 11, 2017

amanmahajan7 deleted the dj-grouping-by-column-name branch March 14, 2019 21:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

group by with object array and string array #857

group by with object array and string array #857

d4nj0nes commented Jun 29, 2017

coveralls commented Jun 29, 2017

coveralls commented Jun 29, 2017

diogofcunha left a comment

diogofcunha Jun 30, 2017

d4nj0nes Jun 30, 2017

diogofcunha Jun 30, 2017

d4nj0nes Jun 30, 2017

d4nj0nes Jun 30, 2017

diogofcunha Jun 30, 2017 •

edited

d4nj0nes Jun 30, 2017

diogofcunha Jun 30, 2017

d4nj0nes Jun 30, 2017

diogofcunha Jul 3, 2017

d4nj0nes Jul 3, 2017

coveralls commented Jun 30, 2017

diogofcunha Jul 3, 2017

coveralls commented Jul 3, 2017

group by with object array and string array #857

group by with object array and string array #857

Conversation

d4nj0nes commented Jun 29, 2017

Description

coveralls commented Jun 29, 2017

coveralls commented Jun 29, 2017

diogofcunha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diogofcunha Jun 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 30, 2017

Choose a reason for hiding this comment

coveralls commented Jul 3, 2017

diogofcunha Jun 30, 2017 •

edited