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] Deprecate the closePopover callback passed to cellActions in favor of closeCellPopover ref API #5734

Merged
merged 3 commits into from
Apr 5, 2022

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Mar 18, 2022

Summary

This PR essentially deprecates/reverts #4346 in favor of the closeCellPopover ref API added in #5590/#5550.

The removed 'email' doc example is actually broken in prod - I apparently forgot to pass closePopover to the popover cell actions when I was refactoring 🙈 After chatting with @chandlerprall, we decided that instead of fixing it, it was best to reduce confusion for consumers by favoring 1 API over having 2 ways to close cell popovers.

Checklist

@kibanamachine
Copy link

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

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.

Code changes look good.

Would you mind building eui.d.ts and testing it in Kibana? Should help identify any uses over there that have been affected.

@cee-chen
Copy link
Contributor Author

For sure, great call!

@cee-chen
Copy link
Contributor Author

yarn kbn bootstrap --no-validate && yarn start ran with no errors! 🤯 I also ran yarn tsc just in case and nothing. So I'm guessing either folks in Kibana weren't using closePopover or are overriding EuiDataGrid's types with their own.

@cee-chen
Copy link
Contributor Author

cee-chen commented Mar 22, 2022

Ah wait, turns out I was running the wrong command(s). node scripts/type_check.js was the one I wanted. Here's the output of it:

 proc [tsc] src/plugins/discover/public/components/discover_grid/discover_grid_cell_actions.test.tsx:46:11 - error TS2322: Type '{ Component: (props: any) => Element; rowIndex: number; colIndex: number; columnId: string; isExpanded: false; closePopover: Mock<any, any>; }' is not assignable to type 'IntrinsicAttributes & EuiDataGridColumnCellActionProps'.
 proc [tsc]   Property 'closePopover' does not exist on type 'IntrinsicAttributes & EuiDataGridColumnCellActionProps'.
 proc [tsc] 
 proc [tsc] 46           closePopover={jest.fn()}
 proc [tsc]              ~~~~~~~~~~~~
 proc [tsc] 
 proc [tsc] src/plugins/discover/public/components/discover_grid/discover_grid_cell_actions.test.tsx:75:11 - error TS2322: Type '{ Component: (props: any) => Element; rowIndex: number; colIndex: number; columnId: string; isExpanded: false; closePopover: Mock<any, any>; }' is not assignable to type 'IntrinsicAttributes & EuiDataGridColumnCellActionProps'.
 proc [tsc]   Property 'closePopover' does not exist on type 'IntrinsicAttributes & EuiDataGridColumnCellActionProps'.
 proc [tsc] 
 proc [tsc] 75           closePopover={jest.fn()}
 proc [tsc]              ~~~~~~~~~~~~
 proc [tsc] 
 proc [tsc] src/plugins/vis_types/table/public/components/table_vis_columns.tsx:58:45 - error TS2339: Property 'closePopover' does not exist on type 'EuiDataGridColumnCellActionProps'.
 proc [tsc] 
 proc [tsc] 58           ({ rowIndex, columnId, Component, closePopover }: EuiDataGridColumnCellActionProps) => {
 proc [tsc]                                                ~~~~~~~~~~~~
 proc [tsc] 
 proc [tsc] src/plugins/vis_types/table/public/components/table_vis_columns.tsx:95:45 - error TS2339: Property 'closePopover' does not exist on type 'EuiDataGridColumnCellActionProps'.
 proc [tsc] 
 proc [tsc] 95           ({ rowIndex, columnId, Component, closePopover }: EuiDataGridColumnCellActionProps) => {
 proc [tsc]                                                ~~~~~~~~~~~~
 proc [tsc] 
 proc [tsc] x-pack/plugins/lens/public/datatable_visualization/components/columns.tsx:76:47 - error TS2339: Property 'closePopover' does not exist on type 'EuiDataGridColumnCellActionProps'.
 proc [tsc] 
 proc [tsc] 76             ({ rowIndex, columnId, Component, closePopover }: EuiDataGridColumnCellActionProps) => {
 proc [tsc]                                                  ~~~~~~~~~~~~
 proc [tsc] 
 proc [tsc] x-pack/plugins/lens/public/datatable_visualization/components/columns.tsx:114:47 - error TS2339: Property 'closePopover' does not exist on type 'EuiDataGridColumnCellActionProps'.
 proc [tsc] 
 proc [tsc] 114             ({ rowIndex, columnId, Component, closePopover }: EuiDataGridColumnCellActionProps) => {
 proc [tsc]                                                   ~~~~~~~~~~~~
 proc [tsc] 

So looks like just 2 plugins with production changes to close the popover. I looked at the them more closely and I'm pretty sure I can see where to make those changes and pass ref.closeCellPopover to the file.

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.

Changes LGTM 🚢

@cee-chen cee-chen enabled auto-merge (squash) April 5, 2022 17:08
@kibanamachine
Copy link

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

@cee-chen cee-chen merged commit 6d8f03b into elastic:main Apr 5, 2022
@cee-chen cee-chen deleted the datagrid/close-popover branch April 5, 2022 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants