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] Feature: toolbar reorganization and row height control #5415

Merged
merged 19 commits into from
Dec 7, 2021

Conversation

cee-chen
Copy link
Contributor

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

Summary

closes #5080

This PR re-organizes the toolbar into distinct left/right sections (the left generally handling data control such as sorting, and the right side handling display controls such as density and full-screen). Right side controls are also now icons with tooltips rather than full buttons (to help with space constraints).

Next to the density control now exists a new feature, the row height control. This allows users to switch between single lines, auto fit, and custom line counts.

A new 'Reset to default' button has also been added that allows users to switch back to the original styles/row heights set on grid init, and onChange callbacks have been added that allows devs to save grid style/row height state to (e.g.) localStorage.

demo1

demo2

API changes

  • toolbarVisibility.showStyleSelector has been deprecated. It is now showDisplaySelector, with the following options:
    • { allowDensity?: boolean; allowHide?: boolean; }
  • toolbarVisibility.additionalControls now supports an object configuration of positions:
    • { left?: { prepend?: ReactNode; append?: ReactNode }; right?: ReactNode; }
    • If passed a single ReactNode like its previous API, controls will be be automatically added into the left.append position (was previously left.prepend)
  • gridStyle and rowHeightsOptions both now accept an onChange key/callback that will fire with the current internal grid state when the user interacts with the display controls.

Individual feature branch PRs have been reviewed and their history can be seen here:

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs and playground toggles
  • Added documentation
  • Checked Code Sandbox works for any docs examples
  • Added or updated jest and cypress 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

Constance and others added 8 commits November 10, 2021 14:24
* [Setup] Move toolbar CSS to own file

* [Cleanup] DRY out repeated specific CSS class

- naming was incorrect in the case of the style selector popover

* Reorder toolbar per Caroline's mocks

- Columns & Sort on left, with additionalControls to the right of those (+ update prop types to indicate that)

- Style (TBD naming) and Fullscreen to the right

* Convert display buttons to icons only

+ tweak grid styles icon to match Caroline's mock + popover location

* Convert density UI to a compressed EuiFormRow w/ new text/order per mocks

- refactor densityOptions to const outside hook

* Update test snapshots

* Add changelog entry

* [PR feedback] Revert location of additionalControls + fix props description

* [PR feedback] CSS feedback

* [PR feature request] Change toolbar icon based on grid density

* [PR feedback][extra] Fix missing sort control on styling example

NB: sorting functionality won't actually work/isn't hooked up, but this should at least allow the show/hide UI toggle to work as expected

* updoot snaps

* Fix changelog

* [misc] Reorganize EuiDataGridToolbar utils to bottom of file

- makes it easier for devs to immediately jump to the main component/fn, rather than have to scroll past helper/minutae details

- if it gets complex enough, also worth pulling out to a separate file

* Rename CSS toolbar class to left/right to match API

- Per GH discussion display/data is ambiguous and naming this left/right is likely more helpful

* Add API support for left/right positioning of `additionalControls`

* Update additionalControls documentation + example

* PR feedback & cleanup from Caroline (#4)

* Update some props comments and fix popover’s responsiveness

* [Design] Remove a few extraneous style overrides to control buttons

And lower left padding

* Fixed add/remove of fullscreen body class

* Added tooltips around icon buttons with long delay

* Updated examples and snippets with “real world” examples

Removed all the unnecessary manual additions of `className="euiDataGrid__controlBtn”`

* More responsive fixes

* Change class name

* Update CSS class to be more specific

- It isn't necessary for popovers without drag & drop, name becomes verbose but it's hopefully worth it for clearer usage

* closes #5092 - add changelog for fullscreen bugfix in Caroline's work

* [PR feedback] Changelog

* Update snapshots

* PR feedback

* Remove unused CSS class

- Class was removed in a previous commit by Caroline

Co-authored-by: Caroline Horn <[email protected]>
…ols` API (#5394)

* Split up left toolbar positions into optional left.prepend and left.append config

* Update documentation

* Update changelog entry

* PR feedback
* Rename styleSelector to displaySelector + update types & docs

* Refactor out nested object helper

- so that both the columnSelector and displaySelector can use it

* Update displaySelector with conditional density/row height options

+ update docs:
  - reorder UI to match toolbar layout
  - fix legends

+ add unit tests (& improve some existing unit tests)

* Add rowHeightsOptions controls to popover

NYI: conditional lineCount controls

+ unit tests

* Add conditional lineCount row and logic
+ fix cell height to dynamically on lineCount change

* Fix styles not applying correctly for rowHeightsOptions that have undefined heights but defined rowHeightsOptions objects

e.g. - empty objs, or lineHeight set but nothing else

+ simplify this.props

* Fix multiline content not top-aligning correctly when in single/undefined mode

- Multiline content includes items with `<br>`/`<p>` tags etc that end up incorrectly vertically centered to their increased height

* Fix single/undefined row heights to account for passed lineHeight API + density

- by giving it the same setRowHeight logic as line counts with a single line

+ rename recalculateLineCountHeight for clarity now that it's no longer being used for just lineCount

* Add changelog entry

* [PR feedback] Increase default lineCount to 2

- to make the difference more obvious when switching between single and custom

* Fix changelog

* Fix control column appearance by switching them back to vertical centering

* Tweak various height alignments on compressed grid settings

- expand buttons on single lines should now be more vertically aligned

- The manual button/checkbox changes for control columns is somewhat required for auto height to not be dramatically different for single lines

* Fix expand action button background color mismatch for auto/lineCount rows
- when hovering over rows
- on striped rows

- Not caused by this PR, but this was really annoying me, haha

* PR feedback: convert cell actions CSS selectors to mixin

- Slightly less grody to look at in-code, and allows us to provide more comments for context

The more I look at __expandButton the more I dislike the naming also but it would involve too many changes right now to rename button->actions, so I added a TODO

* Fix sasslint issues

* PR feedback: Change line count number to EuiRange

+ Remove ability to set negative or zero numbers, which causes the grid to flip out

* PR feedback - remove height tweaks for compressed auto fit

* [PR feedback] Remove `showStyleSelector`

* Remove unused unit tests

- This should have been done in 79c878d, but I missed it

* [PR feedback] Intelligently disable toolbar control if all nested options are disabled

Notes:
- Strongly recommend turning off whitespace changes to reduce indentation noise
- This could have been done at the `data_grid_toolbar` level and could be a microperf optimization to do so, but since this is a relatively unlikely edge case in any case I don't think it's worth the overoptimization when this approach is relatively straightforward to implement

* Do not fall back to undefined/single for static heights

* PR changelog feedback

Co-authored-by: Caroline Horn <[email protected]>

* Update grid density to also intelligently detect initial state

+ not set a button state if custom fontSize/cellPadding don't match our defined density states

* [PR feedback] Improve typing to avoid any usage

+ improve unit tests to cover undefined nested object key case

* [PR feedback] Document recommendation for disabling row height switcher when using `rowHeights`

@see #5411

* [Pr feedback] Simplify getNestedObjectOptions

* Add workaround for failing Cypress tests

Also see https://stackoverflow.com/questions/49384120/resizeobserver-loop-limit-exceeded

Co-authored-by: Caroline Horn <[email protected]>
@cee-chen
Copy link
Contributor Author

cee-chen commented Nov 24, 2021

I figured this feature branch is at least ready to open up, although I think think there are a few remaining questions maybe worth addressing before we would hit merge (if so, I can move this back into a draft PR).

Remaining question 1:

rowHeights overrides - addressed in #5428

Remaining question 2:

onChange callbacks - addressed in #5424

Remaining bug 3:

#5406 - addressed in #5447

Others?

Are there any remaining features or questions to address as part of this feature branch? Leave a comment if so!

@kibanamachine
Copy link

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

Copy link
Contributor

@elizabetdev elizabetdev left a comment

Choose a reason for hiding this comment

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

Thanks @constancecchen. It's looking good! 🎉

I just have a few comments.

@kibanamachine
Copy link

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

@cee-chen cee-chen force-pushed the feat/datagrid/toolbar-reorg-and-row-height-switcher branch from 3022d9f to 2848d56 Compare November 30, 2021 20:50
@kibanamachine
Copy link

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

@elizabetdev elizabetdev self-requested a review December 2, 2021 15:01
Copy link
Contributor

@elizabetdev elizabetdev left a comment

Choose a reason for hiding this comment

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

Thanks for going through the suggestions. LGTM! 🎉

Are there any remaining features or questions to address as part of this feature branch? Leave a comment if so!

All fine to me! Really excited for the upcoming reset button. 😄

Constance and others added 2 commits December 2, 2021 09:20
…5424)

* Add new useUpdateEffect hook

- essentially just useEffect that fires only on update/rerender and not on mount
- inspiration from https://github.com/streamich/react-use/blob/master/docs/useUpdateEffect.md

+ update useDependentState to dogfood hook (since it's essentially using the same ref/mount logic that I want)

* Add `onChange` callbacks to `gridStyles` and `rowHeightsOptions`

 - and fire said callbacks on update whenever user configurations change

* Add documentation example
 - with density responsiveness & localStorage

* Add changelog entry

* [PR feedback] Adjust callback demo to load stored configs on SPA navigation
@kibanamachine
Copy link

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

…reset button (#5428)

* Unset specific `rowHeights` when using the row height switcher

+ lineCount changes to also unset rowHeights overrides

* Fix rowHeights snippet to actually include rowHeights

+ improve snippet label

* [??] Remove toolbarVisibility.showDisplaySelector configs

- to allow for testing
- also since this is documented behavior, I don't know if need this in the actual example anymore - but if people prefer it, I can revert this commit

* [Misc documentation cleanup] Remove unrequired props from snippets

- to help focus more on rowHeightsOptions and more easily provide a bare minimum example

* Add reset to initial state button/logic

* [PR feedback] Conditionally render reset button

* [PR feedback] Right align button

Co-authored-by: Caroline Horn <[email protected]>

* Fix closing tag

* fix imports

* Update src/components/datagrid/controls/display_selector.tsx

Co-authored-by: Caroline Horn <[email protected]>

* No `grow`

Co-authored-by: Caroline Horn <[email protected]>
Co-authored-by: cchaos <[email protected]>
@kibanamachine
Copy link

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

@cee-chen
Copy link
Contributor Author

cee-chen commented Dec 7, 2021

Once #5447 merges in, I'll rebase this feature branch against latest main, wait for the CI preview link, do some final QA/testing, and then merge it in!

Thanks so much @chandlerprall @cchaos and @miukimiu for all your incredible guidance and work on this! 🎉

chandlerprall and others added 2 commits December 7, 2021 12:49
* Calculate the grid's height based on all known values

* Add cypress test to verify switching from default single row height to auto fit will expand the grid

* remove comment

* Compute the grid's unconstrained height when a row's height changes

* Simplify setRenderPass logic

* Added useForceRender hook, refactored data grid row height updating to use the new hook

* Created a hook to better organize and document how the unconstrained height is calculated

* Update src/services/hooks/useForceRender.ts

Co-authored-by: Constance <[email protected]>

Co-authored-by: Constance Chen <[email protected]>
Co-authored-by: Constance <[email protected]>
@kibanamachine
Copy link

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

@cee-chen
Copy link
Contributor Author

cee-chen commented Dec 7, 2021

Just re-ran QA/checklist steps against the latest deployed staging and everything looks great! 🤩 Going to turn on auto-merge here so this gets in sooner rather than later for today's release. 🚂

@kibanamachine
Copy link

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

@cee-chen cee-chen enabled auto-merge (squash) December 7, 2021 20:59
@kibanamachine
Copy link

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

@cee-chen cee-chen merged commit b97f93e into main Dec 7, 2021
@cee-chen cee-chen deleted the feat/datagrid/toolbar-reorg-and-row-height-switcher branch December 7, 2021 22:02
@kibanamachine
Copy link

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

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.

[EuiDataGrid] Add row height switcher to column
4 participants