Terra-core: [Table] - HeaderCell onClick prop is overwritten

[StephenEsser]

Issue Description

Consumers wanting to implement sorting need to be able to pass an onClick to a Table's HeaderCell. Currently the HeaderCell's onClick is always overwritten by the click event of the Table.Header

https://github.com/cerner/terra-core/blob/master/packages/terra-table/src/TableHeader.jsx#L33

Issue Type

• [ ] New Feature
• [ ] Enhancement
• [X] Bug
• [ ] Other

Expected Behavior

A Table.HeaderCell should honor an onClick directly passed.

Current Behavior

A Table.HeaderCell's onClick is always overwritten by the onClick of the Table.Header.

 <Table.Header>
    <Table.HeaderCell content={'Name'} minWidth="small" sort="desc" onClick={() => console.log('This onClick will never be invoked')} />
    <Table.HeaderCell content={'Address'} key={'ADDRESS'} minWidth={'medium'} />
    <Table.HeaderCell content={'Phone Number'} key={'PHONE_NUMBER'} minWidth={'large'} />
 </Table.Header>

Steps to Reproduce

1. Create a Table
2. Add an onClick to a Table.HeaderCell
3. Observe the onClick is always overwritten,

Environment

• All environments

[dylanklohr]

@StephenEsser , looking at the line you posted, and tracking it up to here where newProps is declared, would you expect the behavior to allow for both onClicks to be fired (both the Table.Header's and the Table.HeaderCell's, or only the Table.HeaderCell's, when provided?

I ask because in the former case, we could do something like this (Kent C. Dodds provides an explanation of doing this in terms of a prop getter being used with the render prop pattern. But, in the latter, we could modify the line you linked above to be

return React.cloneElement(
  child,
  [...newProps, child.props.onClick]
);

(I believe, this would need to be tested) and essentially ensure that the Table.HeaderCell's onClick is the one that will always _win_ when the props get hashed as they're passed into the new element.

[StephenEsser]

We'll need to determine why the code was implemented this way to uncover if this was intentional functionality or an accidental bug. My assumption is this was written as a workaround to avoid a lint error when adding an onClick to a <thead /> tag.

I would expect both onClicks to be fired if the user added an onClick to both the Table.Header and the Table.HeaderCell. ( Although I think this is a rare use case, but one the consumer can handle )

Click events should bubble. Clicking any child of the Table.Header will invoke an onClick on the Table.Header naturally through event bubbling / propagation without doing the cloning logic here.

Something along these lines is what I had in mind for this change.

const TableHeader = ({ children, ...customProps }) => {

  let childrenArray = React.Children.toArray(children);
  if (childrenArray.length > 16) {
    // eslint-disable-next-line no-console
    console.log(`Number of Columns are ${childrenArray.length}. This is more than columns limit`);
    childrenArray = childrenArray.slice(0, 16);
  }

  return (
    <thead {...customProps}>
      <tr>
        {childrenArray}
      </tr>
    </thead>
  );
};

[jfewins]

@bjankord This defect will block some table sorting work that I have planned for Q2. Do you plan on having a resolution within Q1?

[bjankord]

Resolved in #1376

[bjankord]

This is available in [email protected]