Skip to content

[EuiDataGrid] Row height switcher should override rowHeights + add reset button - #5428

Merged
cee-chen merged 12 commits into
elastic:feat/datagrid/toolbar-reorg-and-row-height-switcherfrom
cee-chen:datagrid-rowheights-override
Dec 3, 2021
Merged

[EuiDataGrid] Row height switcher should override rowHeights + add reset button#5428
cee-chen merged 12 commits into
elastic:feat/datagrid/toolbar-reorg-and-row-height-switcherfrom
cee-chen:datagrid-rowheights-override

Conversation

@cee-chen

@cee-chen cee-chen commented Dec 1, 2021

Copy link
Copy Markdown
Contributor

Summary

closes #5411

Per the above discussion/design decisions, this PR now:

  • Causes the row height UI control to unset/override any specific rowHeights (ee1469d)
  • Adds a 'Reset to default' button which allows reverting user changes to the display selector to the original props passed in by developers (9eca2e3)

Before

before.mp4

After

after.mp4

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

- [ ] Checked Code Sandbox works for any docs examples

- [ ] Checked for breaking changes and labeled appropriately

  • 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 December 1, 2021 17:02
@cee-chen
cee-chen force-pushed the datagrid-rowheights-override branch from fe81de3 to 37f3640 Compare December 1, 2021 17:12
+ lineCount changes to also unset rowHeights overrides
- 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
- to help focus more on rowHeightsOptions and more easily provide a bare minimum example
@cee-chen

cee-chen commented Dec 1, 2021

Copy link
Copy Markdown
Contributor Author

Sorry for force pushes (found a bug!), this is ready for review now with before/after screencaps!

@kibanamachine

Copy link
Copy Markdown
Contributor

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

@cchaos cchaos left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

💟 Before, I do a full review, is it possible to display the reset button only if the user has changed the configurations?

@kibanamachine

Copy link
Copy Markdown
Contributor

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

@cee-chen

cee-chen commented Dec 2, 2021

Copy link
Copy Markdown
Contributor Author

Sure - 9b70058

@kibanamachine

Copy link
Copy Markdown
Contributor

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

@cchaos cchaos left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🙌 Works wonderfully. Just a visual suggestion.

Comment thread src/components/datagrid/controls/display_selector.tsx
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
@cchaos

cchaos commented Dec 2, 2021

Copy link
Copy Markdown
Contributor

Jenkins, test this

@cee-chen

cee-chen commented Dec 2, 2021

Copy link
Copy Markdown
Contributor Author

Whoops, thanks for catching that!

@cee-chen

cee-chen commented Dec 2, 2021

Copy link
Copy Markdown
Contributor Author

jenkins test this

Comment thread src/components/datagrid/controls/display_selector.tsx Outdated
@cchaos

cchaos commented Dec 3, 2021

Copy link
Copy Markdown
Contributor

And those components need to be imported...lol

09:42:18 ERROR in components/datagrid/controls/display_selector.tsx:365:14
09:42:18 TS2304: Cannot find name 'EuiFlexGroup'.
09:42:18     363 |         {showResetButton && (
09:42:18     364 |           <EuiPopoverFooter>
09:42:18   > 365 |             <EuiFlexGroup justifyContent="flexEnd" responsive={false}>
09:42:18         |              ^^^^^^^^^^^^
09:42:18     366 |               <EuiFlexItem>
09:42:18     367 |                 <EuiButtonEmpty
09:42:18     368 |                   flush="both"

@cee-chen

cee-chen commented Dec 3, 2021

Copy link
Copy Markdown
Contributor Author

🤦‍♀️ HAHA sorry I definitely should have opened this in VSCode instead of just committing the suggestion. Doing so now!

@cee-chen
cee-chen requested a review from cchaos December 3, 2021 17:06
@kibanamachine

Copy link
Copy Markdown
Contributor

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

Comment thread src/components/datagrid/controls/display_selector.tsx Outdated
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
@kibanamachine

Copy link
Copy Markdown
Contributor

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

@cchaos cchaos left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I need to stop being lazy and just push you a commit 😆 . Pushing a fix to get it to align to the right instead of left. Other than that the rest of the design code LGTM!

@cee-chen

cee-chen commented Dec 3, 2021

Copy link
Copy Markdown
Contributor Author

🤦‍♀️ Haha sorry, i definitely should have done the same. Thanks Caroline!

@kibanamachine

Copy link
Copy Markdown
Contributor

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

@cee-chen

cee-chen commented Dec 3, 2021

Copy link
Copy Markdown
Contributor Author

Preview looks good, merging

@cee-chen
cee-chen merged commit cedee33 into elastic:feat/datagrid/toolbar-reorg-and-row-height-switcher Dec 3, 2021
@cee-chen
cee-chen deleted the datagrid-rowheights-override branch December 3, 2021 22:26
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 <549577+cchaos@users.noreply.github.com>
Co-authored-by: Chandler Prall <chandler.prall@gmail.com>
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