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

[Data Grid] Allow adding custom actions to DataGridCells and cell popover #3668

Merged

Conversation

kertal
Copy link
Member

@kertal kertal commented Jun 29, 2020

Summary

This PR allows to specify custom actions for data grid cell by column. These actions are displayed when the mouse hovers the cell and clickable, and also displayed when the cell gains keyboard focus and can be triggered by keyboard actions in the popover.

Action triggered by mouse click

image

Action triggered by keyboard enter

image

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Firefox and Safari
  • Props have proper autodocs
  • Added documentation
    - [ ] Checked Code Sandbox works for the any docs examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes~
  • A changelog entry exists and is marked appropriately

@kertal kertal self-assigned this Jun 29, 2020
@kertal kertal marked this pull request as draft June 29, 2020 11:07
@kibanamachine
Copy link

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

@kertal
Copy link
Member Author

kertal commented Jun 29, 2020

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

this is fantastic!

@snide
Copy link
Contributor

snide commented Jul 2, 2020

@kertal I noticed you have this in draft mode? Do you want a review?

@kertal
Copy link
Member Author

kertal commented Jul 2, 2020

@kertal I noticed you have this in draft mode? Do you want a review?

thx @snide currently testing this in the Discover PR which might lead to a change, but I'm out tomorrow, so let's finish it next week,

@kertal
Copy link
Member Author

kertal commented Jul 8, 2020

@kertal I noticed you have this in draft mode? Do you want a review?

thx @snide currently testing this in the Discover PR which might lead to a change, but I'm out tomorrow, so let's finish it next week,

@snide I've experienced a performance when using this PR's modification in my Discover PR in Chrome. Start of the week it was gone, but now I know why, been using my non-retina monitor to debug it, so here is the issue how to reproduce it in the current EUI version: #3705

@kibanamachine
Copy link

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

@kertal kertal marked this pull request as ready for review August 4, 2020 14:21
@kertal
Copy link
Member Author

kertal commented Aug 4, 2020

since #3726 was merged, performance is fine, so this is now ready for review

@cchaos cchaos self-requested a review August 18, 2020 16:59
@cchaos
Copy link
Contributor

cchaos commented Aug 18, 2020

I can get this into a weird state (maybe it's like a fake focused state or something) where, if I click the button to open the popover, then hit esc, It's still showing the popout button but without any space between it and the content until I hover over it.
Screen Recording 2020-08-18 at 03 28 44 PM

@kibanamachine
Copy link

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

@kertal
Copy link
Member Author

kertal commented Aug 20, 2020

I can get this into a weird state (maybe it's like a fake focused state or something) where, if I click the button to open the popover, then hit esc, It's still showing the popout button but without any space between it and the content until I hover over it.

@cchaos I can't reproduce (tested in the Preview documentation change), in which browser do you experience this problem?

@cchaos
Copy link
Contributor

cchaos commented Aug 25, 2020

I was also on the preview build and using Chrome. I can reliably reproduce this with the following steps:

  1. Hover and click on the popout to see the popover
  2. Hit esc which closes the popover
  3. Without clicking on anything else, just start rolling the mouse over other cells then back to the original cell

@kertal
Copy link
Member Author

kertal commented Aug 26, 2020

@cchaos thx! I could reproduce it now!

@kibanamachine
Copy link

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

@kertal
Copy link
Member Author

kertal commented Aug 27, 2020

I was also on the preview build and using Chrome. I can reliably reproduce this with the following steps:

  1. Hover and click on the popout to see the popover
  2. Hit esc which closes the popover
  3. Without clicking on anything else, just start rolling the mouse over other cells then back to the original cell

So what happens is, that after closing the popover, which owns focus, focuses the expand button, and therefore this is displayed, changed the CSS to prevent this. thx for catching this!

@kertal
Copy link
Member Author

kertal commented Oct 13, 2020

so how would we handle this, if we would use the CustomAction approach?

The provided Component could be a small wrapper around EuiButtonIcon, (props) => <EuiButtonIcon {...props} aria-hidden/>

@chandlerprall I changed and it works! Hopefully last problem: how to satisfy TypeScript for the Component prop? Here's the draft that makes TypeScript cry.

export interface EuiDataGridColumnCellActionProps {
  /**
   * The index of the row that contains cell's data
   */
  rowIndex: number;
  /**
   * The id of the column that contains the cell's data
   */
  columnId: string;
  /**
   * React component representing the action displayed in the cell
   */
  Component: JSXElementConstructor<EuiButtonIconProps | EuiButtonEmptyProps>;
  /**
   * Determines whether the cell's action is displayed expanded (in the Popover)
   */
  isExpanded: boolean;
}

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kertal
Copy link
Member Author

kertal commented Oct 14, 2020

jenkins test this

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

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; pulled & tested locally. 🎉

Copy link
Contributor

@andreadelrio andreadelrio left a comment

Choose a reason for hiding this comment

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

@kertal Row counter is starting at 0 for "paint", sent a suggestion.

src-docs/src/views/datagrid/column_cell_actions.js Outdated Show resolved Hide resolved
src-docs/src/views/datagrid/column_cell_actions.js Outdated Show resolved Hide resolved
@andreadelrio andreadelrio self-requested a review October 19, 2020 20:03
Copy link
Contributor

@andreadelrio andreadelrio left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@kibanamachine
Copy link

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

@kertal kertal merged commit 5226ddb into elastic:master Oct 20, 2020
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.

7 participants