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

feat(react-grid): provide the custom data accessors capability #264

Merged

Conversation

SergeyAlexeev
Copy link
Contributor

No description provided.

@SergeyAlexeev SergeyAlexeev changed the title feat(react grid): add the functionality to use nested keys for column definition feat(react-grid): add the functionality to use nested keys for column definition Aug 15, 2017
@dxbykov dxbykov changed the title feat(react-grid): add the functionality to use nested keys for column definition feat(react-grid): provide the custom data accessors capability Aug 15, 2017
@@ -16,3 +16,19 @@ export const addedRowsByIds = (addedRows, rowIds) => {
});
return result;
};

export const rowChange = (columns, createRowChange) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Confusing method name

title: 'First Name',
getCellData: row => (row.user ? row.user.firstName : undefined),
createRowChange: (row, columnName, value) =>
(Object.assign(row.user || { user: {} }, { firstName: value })),
Copy link
Contributor

Choose a reason for hiding this comment

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

In case of row.user is undefined the result will look as { user: {}, firstName: value }



Pay attention, the `getCellData` property of the Grid has a higher priority than the `column.getCellData` one.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would also make a note, that it's a performance-critical place. Since the getCellData method is called lots of times it should be implemented in an optimal way. This demo code shouldn't be used in productions apps with a large amount of data.

{
...
createRowChange: (row, columnName, value) =>
(Object.assign(row.user || { user: {} }, { firstName: value })),
Copy link
Contributor

Choose a reason for hiding this comment

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

Same issue as mentioned above.

@@ -105,6 +106,7 @@ export class EditingState extends React.PureComponent {
const changedRows = this.props.changedRows || this.state.changedRows;
const addedRows = this.props.addedRows || this.state.addedRows;
const deletedRows = this.props.deletedRows || this.state.deletedRows;
const createRowChange = this.props.createRowChange || this.state.createRowChange;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can createRowChange be in the state?

return {
value: column.name in change ? change[column.name] : row[column.name],
value: getCellData(change, column.name) || getCellData(row, column.name),
Copy link
Contributor

Choose a reason for hiding this comment

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

if change = { user: undefined } and row = { user: 'A user' }, then the value will be 'A user' but should be undefined.

@@ -34,6 +35,8 @@ A value with the following shape:
Field | Type | Description
------|------|------------
name | string | Specifies the field name in the data row to obtain a column value. A unique key can also be used to identify a column
getCellData | (row: [Row](#row), columnName: string) => any | Specifies the function used to get a cell data
createRowChange | (row: [Row](#row), columnName: string, value: string | number) => object | Specifies the function used to create a row change
Copy link
Contributor

Choose a reason for hiding this comment

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

createRowChange should be defined in EditingState's Column interface extension

Copy link
Contributor

Choose a reason for hiding this comment

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

Also seems that the columnName argument should be the last because it will not be frequently used

@SergeyAlexeev SergeyAlexeev merged commit 5f699bf into DevExpress:master Aug 21, 2017
@SergeyAlexeev SergeyAlexeev deleted the nested-keys-implementation branch August 21, 2017 06:58
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.

6 participants