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] Add extra left prepend position to the additionalControls API #5394

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Nov 19, 2021

Summary

Per Caroline's comment here, the Discover team requires the being able to put additional controls to the left of our columns/sort selectors:

This PR splits up the left position key an object allowing left.append and left.prepend. Single nodes passed in instead of an object of positions will default to left.append.

New documentation example:

Checklist

- [ ] Check against all themes for compatibility in both light and dark modes
- [ ] Checked in mobile
- [ ] Checked in Chrome, Safari, Edge, and Firefox

- [ ] Checked Code Sandbox works for any docs examples

  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately

This is a breaking API change, but is being merged into a feature branch.

- [ ] Checked for accessibility including keyboard-only and screenreader modes

  • A changelog entry exists and is marked appropriately

@cee-chen cee-chen requested a review from cchaos November 19, 2021 17:49
@cee-chen cee-chen changed the title [EuiDataGrid] Add extra left prepend position additionalControls API [EuiDataGrid] Add extra left prepend position to the additionalControls API Nov 19, 2021
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Quick clarification from the docs:

Use the toolbarVisibility.additionalControls prop to pass more buttons to the toolbar. It will respect the toolbarVisibility={false} setting and hide when appropriate.

Would that even be possible? Since additionalControls is nested within toolbarVisibility you cant have toolbarVisibility = {false & additionalControls: {}}.


The component work LGTM, my comments mostly revolve around the docs:

I'd change the sentence:

Passing a single node to additionalControls will default to being appended to the left side of the toolbar.

to:

Passing a single node to additionalControls will default to being placed in the left.append position of the toolbar.

Edit: I didn't read the 'appended' portion of the sentence. I've adjusted my suggestion.


It also might be helpful to add into the window.alert which configuration was used to place that control.

CHANGELOG.md Outdated Show resolved Hide resolved
@cee-chen
Copy link
Contributor Author

Use the toolbarVisibility.additionalControls prop to pass more buttons to the toolbar. It will respect the toolbarVisibility={false} setting and hide when appropriate.

Would that even be possible? Since additionalControls is nested within toolbarVisibility you cant have toolbarVisibility = {false & additionalControls: {}}.

Yeah, it does seem like a no brainer since there's no way to define additionalControls if toolbarVisibility is set to false. I can remove that line if you think it's not particularly helpful!

@cee-chen
Copy link
Contributor Author

Feedback should be addressed in latest commit!

@kibanamachine
Copy link

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

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

👍 Design and design code LGTM. Checked in Safari and Firefox too.

@cee-chen cee-chen merged commit 72fdf3f into elastic:feat/datagrid/toolbar-reorg-and-row-height-switcher Nov 23, 2021
@cee-chen cee-chen deleted the datagrid-left-append branch November 23, 2021 21:32
cee-chen pushed a commit that referenced this pull request Dec 7, 2021
…5415)

* [EuiDataGrid] Toolbar UI layout reorganization (#5334)

* [EuiDataGrid] Add extra left prepend position to the `additionalControls` API (#5394)

* [EuiDataGrid] Add rowHeightSwitcher logic + API change (#5372)

* [EuiDataGrid] Add `onChange` callbacks for display selector changes (#5424)

* [EuiDataGrid] Row height switcher should override `rowHeights` + add reset button (#5428)

* [EuiDataGrid] improve height calculation (#5447)

Co-authored-by: Caroline Horn <[email protected]>
Co-authored-by: Chandler Prall <[email protected]>
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.

3 participants