Allow DataGrid's columns popover to optionally not allow hiding and/or reordering by chandlerprall · Pull Request #2993 · elastic/eui

chandlerprall · 2020-03-05T22:29:20Z

Summary

changed text of the column popover button from "Hide fields" / "fields hidden" to "Columns" / "columns hidden". Completely open to further changes. Also, the icon doesn't make sense now @snide
extended toolbarVisibility.showColumnSelector to allow objects as well. I think this nicely and naturally extends the existing design of the toolbarVisibility value, where false values disable, undefined/true values enable, and an object customizes further
created allowHide and allowReorder values on toolbarVisibility.showColumnSelector; this breaks with the existing show* pattern for naming. Do we care?

@snide would you mind doing a design pass?

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

…showColumnSelector

kibanamachine · 2020-03-05T22:46:48Z

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

myasonik · 2020-03-09T13:29:49Z

Maybe we need a few states for this? What are y'all's thoughts on:

Only sort
- Icon: a horizontal version of the sort icon
- Text: "Sort columns" / "𝑛 columns sorted"
Only visibility controls
- Icon: closed eye (what we have today)
- Alternate icon proposal: could also abuse the kqlOperand or merge icons to serve this purpose
- Text: "Hide fields" / "𝑛 fields hidden" (what we have today)
- Alternate text proposal: use "columns" instead of "fields" to keep this button always saying "columns"
Both options available
- Icon: controls (horizontal or vertical)
- Text: "Configure columns" (no "configured" text)
- Tooltip option: Could show a tooltip on the button with "𝑛 columns sorted" and/or "𝑛 fields hidden" as needed

src/components/datagrid/column_selector.tsx

myasonik · 2020-03-09T13:32:57Z

created allowHide and allowReorder values on toolbarVisibility.showColumnSelector; this breaks with the existing show* pattern for naming. Do we care?

I think it's fine. If we decide we do care, I'd propose actually changing all the existing show* props to be allow* props. Just glancing through the docs, it seems like it would work for all of them.

thompsongl · 2020-03-10T19:39:14Z

I'm going to wait for @snide to do his design pass before reviewing. Especially given @myasonik's ideas, which could change the code a bit if implemented.

snide · 2020-03-10T20:15:27Z

Pushed some design changes. Fixed the various states and icons.

Whether or not you can or can not hide columns, we want to keep this button fairly singular in focus. So I kept the "columns" naming and changed the icon to something that works a little more generically. I also fixed the pluralization. Doing a quick docs change, but this should be safe for review now.

I also fixed the styling changes when the label is not used.

snide · 2020-03-10T20:17:03Z

Screens for the lazy

kibanamachine · 2020-03-10T20:30:25Z

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

myasonik · 2020-03-10T21:06:01Z

src/components/datagrid/column_selector.tsx

+          className={controlBtnClasses}
+          data-test-subj="dataGridColumnSelectorButton"
+          onClick={() => setIsOpen(!isOpen)}>
+          {numberOfHiddenFields > 0 ? numberOfHiddenFields : null} {buttonText}


I'm not sure how EUI's i18n system works (or, how it integrates with Kibana) but should we pass the value into the translation string?

Different languages might put the number in a different position or might add a certain count word somewhere in the string as well so we usually want to keep the number and the phrase together.

snide · 2020-03-10T22:12:58Z

The docs are now updated. I moved the demo for this into the toolbar section, which is where I think it belongs. I have a nested state toggle in the popover.

snide

As far as docs, design and functionality go. This is mergible.

thompsongl

should we pass the value into the translation string?

We've been inconsistent at best on this, but it makes sense.

Otherwise, code LGTM 👍

kibanamachine · 2020-03-10T22:29:08Z

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

…d toolbar visibility control to docs

chandlerprall · 2020-03-12T17:02:30Z

Added another nested toggle to the docs for allowing/disabling column reordering, next to the one Dave added for column show/hide capability.

Updated the I18n stuff to place the # of columns hidden value programatically.

snide

Checked locally. Thanks for the i18n fixes.

kibanamachine · 2020-03-12T17:18:03Z

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

thompsongl

Latest commit changes look good 👍

kibanamachine · 2020-03-12T18:42:29Z

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

Added customization functionality to EuiDataGrid's toolbarVisibility.…

1af9555

…showColumnSelector

chandlerprall requested review from myasonik, snide and thompsongl March 5, 2020 22:29

myasonik reviewed Mar 9, 2020

View reviewed changes

src/components/datagrid/column_selector.tsx Outdated Show resolved Hide resolved

snide added the priority label Mar 10, 2020

design updates

67e542d

myasonik approved these changes Mar 10, 2020

View reviewed changes

docs improved

52a3f2b

snide approved these changes Mar 10, 2020

View reviewed changes

thompsongl approved these changes Mar 10, 2020

View reviewed changes

Updated columns localization text to allow moving numeric value; adde…

c5f4aed

…d toolbar visibility control to docs

chandlerprall requested a review from thompsongl March 12, 2020 17:04

snide approved these changes Mar 12, 2020

View reviewed changes

thompsongl approved these changes Mar 12, 2020

View reviewed changes

chandlerprall added 3 commits March 12, 2020 11:52

Merge branch 'master' into feature/2622-datagrid-toolbar-popovers

33c12ca

Changelog & prop docs

fa4df00

Merge branch 'master' into feature/2622-datagrid-toolbar-popovers

89d30be

chandlerprall merged commit 9ce5f85 into elastic:master Mar 12, 2020

chandlerprall deleted the feature/2622-datagrid-toolbar-popovers branch March 12, 2020 18:51

chandlerprall mentioned this pull request Mar 31, 2020

Upgrade to EUI v22.1.1 elastic/kibana#62065

Closed

chandlerprall mentioned this pull request Apr 8, 2020

Upgrade to EUI v22.3.0 elastic/kibana#62963

Merged

Conversation

chandlerprall commented Mar 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

kibanamachine commented Mar 5, 2020

Uh oh!

myasonik commented Mar 9, 2020

Uh oh!

Uh oh!

myasonik commented Mar 9, 2020

Uh oh!

thompsongl commented Mar 10, 2020

Uh oh!

snide commented Mar 10, 2020

Uh oh!

snide commented Mar 10, 2020

Uh oh!

kibanamachine commented Mar 10, 2020

Uh oh!

myasonik Mar 10, 2020

Choose a reason for hiding this comment

Uh oh!

snide commented Mar 10, 2020

Uh oh!

snide left a comment

Choose a reason for hiding this comment

Uh oh!

thompsongl left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Mar 10, 2020

Uh oh!

chandlerprall commented Mar 12, 2020

Uh oh!

snide left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Mar 12, 2020

Uh oh!

thompsongl left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Mar 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

chandlerprall commented Mar 5, 2020 •

edited

Loading