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

Add control columns to EuiDataGrid #2846

Merged

Conversation

chandlerprall
Copy link
Contributor

@chandlerprall chandlerprall commented Feb 12, 2020

Summary

Fixes #2623

Adds control columns to EuiDataGrid for ancillary row interaction. Needs a design pass and tests.

This strongly highlights the bug reported in #2626.

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in IE11 and Firefox
  • Props have proper autodocs
  • Added documentation 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

@chandlerprall chandlerprall changed the title Datagrid/feature/extra columns Add control columns to EuiDataGrid Feb 12, 2020
@snide
Copy link
Contributor

snide commented Feb 12, 2020

I'll handle the design pass on this one today.

@myasonik
Copy link
Contributor

Given that this exacerbates #2626, are we comfortable shipping this without a fix for that?

@snide
Copy link
Contributor

snide commented Feb 12, 2020

Given that this exacerbates #2626, are we comfortable shipping this without a fix for that?

I think it's a separate PR.

@chandlerprall
Copy link
Contributor Author

I agree with Dave - probably best to keep that as a separate PR to keep the overall diff size down, and there's some extra questions&decisions to be had first which are separate from the control column stuff.

@snide
Copy link
Contributor

snide commented Feb 13, 2020

OK @chandlerprall. I fixed the styling issues and cleaned up the docs example to be a little more polished. Some things I did:

  1. I moved the position of additionalControls to the front. I think this makes the most sense from a usage perspective in almost all cases. It's a non-breaking change.
  2. I made it so that control columns don't truncate. Since they require width and should be used for known width elements, I think this is OK. The major benefit here is that it fixes all the focus states (which use outlines and sat outside the overflows).
  3. I adjusted the docs example to give a more designy pattern for how we'd want people to utilize these controls.

As part of this I had to navigate your code a bit and found it easy enough to read. Only comment is that some of it's a little repetitive, but I prefer that to heavy abstraction since this component is already tricky enough.

One feature request (only if easy). I'd prefer that we could pass arbitrary selectors onto the row itself. Similar to setCellProps. That would give us a better way to handle things like highlighting. The example you set up with theming isn't likely something we'd want to do on the design side, and I'd probably prefer not giving people bad ideas in the docs.

@snide snide mentioned this pull request Feb 13, 2020
3 tasks
@cchaos
Copy link
Contributor

cchaos commented Feb 13, 2020

@snide Quick design question. This pattern with using the triple dot icon before a link is new and not really how we've used that icon before.

Screen Shot 2020-02-13 at 10 34 53 AM

Thoughts on why using that instead of our typical down arrow to the right of the link to indicate popovers?

@snide
Copy link
Contributor

snide commented Feb 13, 2020

Thoughts on why using that instead of our typical down arrow to the right of the link to indicate popovers?

No good reason. This isn't baked into the component itself, just the docs. I started with the arrow, but then noticed we were using the dots on the right side so I figured I'd try it out. Likely a bad idea. I'll add the arrow back in.

@snide
Copy link
Contributor

snide commented Feb 13, 2020

Prolly needs a screen for that comment to make sense.

image

@chandlerprall
Copy link
Contributor Author

One feature request (only if easy). I'd prefer that we could pass arbitrary selectors onto the row itself. Similar to setCellProps. That would give us a better way to handle things like highlighting. The example you set up with theming isn't likely something we'd want to do on the design side, and I'd probably prefer not giving people bad ideas in the docs.

Thinking through this, and trying some ideas out.

@snide
Copy link
Contributor

snide commented Feb 13, 2020

Added a flyout example to the docs, which effectively answers the needs of #2462

@chandlerprall chandlerprall marked this pull request as ready for review February 14, 2020 20:44
Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

LGTM; tested locally. One thing to look into if you want.

src/components/datagrid/data_grid_header_cell.tsx Outdated Show resolved Hide resolved
@chandlerprall chandlerprall merged commit 22c983b into elastic:master Feb 19, 2020
@chandlerprall chandlerprall deleted the datagrid/feature/extra-columns branch February 19, 2020 17:30
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.

DataGrid should allow disabling sorting for certain columns
5 participants