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

[EuiDataGrid] Allow isExpandable to be set via setCellProps #5667

Merged
merged 10 commits into from
Mar 2, 2022

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Feb 28, 2022

Summary

closes #2793

This PR allows isExpandable to be set at the cellProps level, which lets consumers override/more granularly set column.isExpandable. Consumers can key off of any of the props passed to renderCellValue, such as rowIndex, colIndex, columnId, schema, etc.

QA

https://eui.elastic.co/pr_5667/#/tabular-content/data-grid-cell-popovers#disabling-cell-expansion-popovers

Checklist

- [ ] Check against all themes for compatibility in both light and dark modes
- [ ] Checked for accessibility including keyboard-only and screenreader modes
- [ ] Checked in mobile
- [ ] Checked in Chrome, Safari, Edge, and Firefox
- [ ] Props have proper autodocs and playground toggles
- [ ] Checked Code Sandbox works for any docs examples

- [ ] Checked for breaking changes and labeled appropriately

  • A changelog entry exists and is marked appropriately

@cee-chen
Copy link
Contributor Author

@chandlerprall 👋 Opening this PR mostly to get your thoughts on this. This PR fully works and solves the use-case that users are requesting in #2793, I just had a few questions for you first:

  • Is this a good idea? Do we want to expand setCellProps to non-DOM-only props / props used by the internal EuiDataGridcell?
  • Or would it be better to follow Chris's suggestion of converting column.isExpandable to accept a fn that returns a boolean instead, so we don't muddy the waters of setCellProps being DOM-only attributes?
  • Do we need a specific documentation example for this, or is this straightforward enough as-is?

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5667/

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

One bit of logic was inverted, but the rest LGTM

@chandlerprall
Copy link
Contributor

Do we need a specific documentation example for this, or is this straightforward enough as-is?

Let's include it on the Data grid cell popovers page

@cee-chen cee-chen marked this pull request as ready for review March 1, 2022 22:50
@cee-chen cee-chen requested a review from chandlerprall March 1, 2022 22:50
@cee-chen cee-chen changed the title [EuiDataGrid] Allow isExpandable to be set at the via setCellProps [EuiDataGrid] Allow isExpandable to be set via setCellProps Mar 1, 2022
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5667/

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

I pushed up a missing useEffect import for the example, but everything else LGTM. I played with the datagrid focus example, verifying overriding from false->true also works and that the various focus states all respect overrides.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5667/

@cee-chen
Copy link
Contributor Author

cee-chen commented Mar 2, 2022

Whoops, I totally forgot to stage that line 🤦‍♀️ (was working ahead to the cell actions branch and getting my staging all dirty). Thanks a million for the catch!

Super fortuitously, the new cell actions logic for #5132 (the part where I need to check if cellActions exists and override isExpandable if so) will be able to hook into the new this.isExpandable() method 🎉

@cee-chen cee-chen merged commit 6ed06df into elastic:main Mar 2, 2022
@cee-chen cee-chen deleted the datagrid/set-is-expandable branch March 2, 2022 01:32
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5667/

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.

[EuiDataGrid] Control isExpandable at the cell level
3 participants