-
Notifications
You must be signed in to change notification settings - Fork 842
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
Add support for custom props for rows in EuiBasicTable and EuiInMemoryTable. #869
Add support for custom props for rows in EuiBasicTable and EuiInMemoryTable. #869
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Was wondering if the current table columns
property is sufficient for allowing custom props at the cell level? e.g if you needed to know the individual cell a user was clicking on what would be the best way of doing it?
@@ -50,7 +50,7 @@ const createUsers = (countries) => { | |||
github: index < 10 ? github[index] : github[index - 10], | |||
dateOfBirth: dob, | |||
nationality: random.oneToOne(countries.map(country => country.code), index), | |||
online: index % 2 === 0 | |||
online: index % 2 === 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - did you mean to add this trailing comma here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep this was deliberate. Good spot!
I'm having second thoughts on this approach. The What if the table accepted a const getRowProps = (row, id, index) => {
const { firstName } = row;
return {
onClick: () => console.log(`clicked ${firstName}`),
'aria-label': `row-${firstName}`,
className: 'customRowClass',
};
};
<EuiBasicTable items={items} rowProps={getRowProps} /> This is a similar pattern to what we already have with Thoughts? |
Great point! Yes I think this would work well. I'll see if I can add this in this PR. |
I like the idea of a |
In ML, we currently only have need for the |
5ef8582
to
bbbe378
Compare
@peteharverson @chandlerprall Ready for another look. I also introduced a breaking change which applies pass-through props to the |
🏆 I just have to point out that all credit for this idea goes to @chandlerprall who first proposed it in #836! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest changes LGTM. I like the way this is done with rowProps
and cellProps
!
@@ -157,8 +158,29 @@ export function getItemId(item, props) { | |||
} | |||
} | |||
|
|||
export class EuiBasicTable extends Component { | |||
function getRowProps(item, rowProps) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should either add rowIndex
here, or remove columnIndex
from getCellProps
(I lean toward removing it from the other function, not sure what value the index provides, and in theory whatever creates getRowProps/getCellProps has context for determining the index).
7a160da
to
775a0c3
Compare
Good point @chandlerprall! Updated. |
…tly to the td element instead of the child div.
e345ff9
to
ed982f4
Compare
Closes #867
This adds support for a
__props__
property on the item which defines a row. Alternatively we could define the row item like this:Though this would be a breaking change, and I think adding pass-through props will be a rarity, so I decided to just create an additional property with the underscores to avoid colliding with any of the other properties.
While I was in this code I also slightly reorganized and simplified the tests. I can extract this to a separate PR if anyone objects.