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

Adds toolbarAdditionalControls prop to EuiDataGrid #2594

Merged
merged 9 commits into from
Dec 9, 2019

Conversation

snide
Copy link
Contributor

@snide snide commented Dec 5, 2019

Summary

Crosses off a line item from #2504

Added a new prop toolbarVisibility.additionalControls which accepts a ReactNode for placing extra buttons within the toolbar. I put these in between the current controls and the full screen for a couple reasons.

  1. The area to the left of the controls will be reserved for eventually adding the selection / action button.
  2. I'd like full screen to always remain on the right, and I'm possibly thinking of making it a simple icon that is right floated to the corner.

Questions

I briefly considered making a component for these buttons, but really all they do is add a class on top of EuiButtonEmpty and i felt like making a shim component for that reason alone seemed like overkill, though if people think that's a good idea to abstract away the css class I can do that.

image

Checklist

  • Checked in dark mode
  • 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

@snide snide requested a review from chandlerprall December 5, 2019 06:12
@snide snide added the data grid label Dec 5, 2019
@snide snide changed the title adds the ability to add aditional toolbar controls to data grid Adds toolbarAdditionalControls prop to EuiDataGrid Dec 5, 2019
@snide snide mentioned this pull request Dec 5, 2019
3 tasks
@chandlerprall
Copy link
Contributor

chandlerprall commented Dec 6, 2019

I wonder if we should merge toolbarVisibility and toolbarAdditionalControls. A single toolbar prop that takes boolean | object to allow disabling, enabling (default), and configuring the [visible] toolbar.

(otherwise this PR LGTM)

@snide
Copy link
Contributor Author

snide commented Dec 6, 2019

@chandlerprall I think that would be elegant. The only downside I can think to that proposal would be:

  1. Certainly more complicated / involved, with likely a lot of moving parts. The prop would accept an array of objects, some of which would be there by default and unchangeable, others would be custom with passed in renders.
  2. Ordering would become less known, and that level of flexibility could lead to people building somewhat different experiences, vs having a set default with a set order + additions.

I think I'd likely need a session to figure out how to handle the typing for something like that if we wanted to go in that direction. I'm personally indifferent from the application perspective, so i'll leave it to you for the call.

@chandlerprall
Copy link
Contributor

Not sure we're envisioning the prop the same way,

toolbar={true} // show toolbar with default configuration (default)
toolbar={false} // disabled
toolbar={{
  additionalControls: <Fragment>...</Fragment>
}}

@snide
Copy link
Contributor Author

snide commented Dec 6, 2019

@chandlerprall Ahhhh. I got you. Thats much easier. I can do that and think that's a good idea!

@snide
Copy link
Contributor Author

snide commented Dec 6, 2019

@chandlerprall Changes made. I kept the toolbarVisibility prop name though because I didn't think it deviated enough in function to make a breaking change to the API.

snide and others added 2 commits December 6, 2019 15:42
@chandlerprall
Copy link
Contributor

Got a PR for you on the typescript side of things, to make the helper utility's return type more explicit. snide#10

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; tested locally

@snide snide merged commit 4adca15 into elastic:master Dec 9, 2019
@snide snide deleted the grid/toolbar_controls branch December 9, 2019 23:05
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.

2 participants