Skip to content

Conversation

jlc467
Copy link

@jlc467 jlc467 commented Nov 13, 2016

For example, if user wants to trigger sort behavior on any click within the th. Can use event to filter unwanted clicks, e.g. filter menu or specific sort direction arrow.

Downside is, within onCellClick, user can't always assume it is a body cell anymore, though It is easy enough to tell the difference between body and head cells.

Also, column key helps user determine the actual cell clicked. As of now, we just know the row and must use the event to know cell. Alternatively, give the user the row and column index like material-ui

e.g. if user wants to to make the entire cell a target for sort toggle
covers th and td
@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) to 83.481% when pulling 7462689 on jlc467:master into e139553 on react-component:master.

make it easier to identify the exact cell clicked.
@jlc467 jlc467 changed the title make onCellClick work for table header cells Add onCellClick for th and column key as argument Nov 13, 2016
@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 84.014% when pulling 5be42e5 on jlc467:master into e139553 on react-component:master.

};
if (column.onCellClick) {
cell.onClick = (e) => column.onCellClick(column, column.key, e);
}
Copy link
Member

Choose a reason for hiding this comment

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

I think the original purpose of onCellClick is only applying to the body cell. If we want apply onCellClick to header cell as well, we should keep the arguments consistent.

/cc @afc163

@yesmeck
Copy link
Member

yesmeck commented Nov 14, 2016

It seems material-ui only applies onCellClick to body cell too. https://github.com/callemall/material-ui/blob/f2edf58f6e1683ce26f5046ce05a6c9e83b60928/src/Table/Table.js#L159-L186

@yesmeck
Copy link
Member

yesmeck commented Nov 14, 2016

Actually, you already know which column occurs event because you are defining onCellClick on that column.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.3%) to 82.124% when pulling 70c166f on jlc467:master into e139553 on react-component:master.

@jlc467
Copy link
Author

jlc467 commented Nov 14, 2016

Will close this for now. If there is demand we can add onColumnClick or something like that.

@jlc467 jlc467 closed this Nov 14, 2016
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.

3 participants