Skip to content

[EuiDataGrid] Add onChange callbacks for display selector changes#5424

Merged
cee-chen merged 5 commits intoelastic:feat/datagrid/toolbar-reorg-and-row-height-switcherfrom
cee-chen:datagrid-onchange
Dec 2, 2021
Merged

[EuiDataGrid] Add onChange callbacks for display selector changes#5424
cee-chen merged 5 commits intoelastic:feat/datagrid/toolbar-reorg-and-row-height-switcherfrom
cee-chen:datagrid-onchange

Conversation

@cee-chen
Copy link
Copy Markdown
Contributor

@cee-chen cee-chen commented Nov 30, 2021 •
edited
Loading

Summary

Per feedback from Caroline and Vlad, we should expose internal gridStyle and rowHeightsOptions state in an onChange callback:

In the future, we should provide a grid example that manually adjusts its content based on the selected display size. Does that display selector have a callback function that consumers can use?
(From Caroline)

I had some thought about height switcher, what we can do if devs will need current state of rowHeightsOptions to store it in something like saved object.
(From Vlad)

After discussion with @chandlerprall, the API we settled on was adding these callbacks in gridStyle.onChange and rowHeightsOptions.onChange.

Screencaps

snapshot.mp4

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

- [ ] 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 commented Nov 30, 2021
View reviewed changes
@cee-chen cee-chen mentioned this pull request Nov 30, 2021
Merged
10 tasks
@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Nov 30, 2021

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

chandlerprall
chandlerprall suggested changes Dec 1, 2021
View reviewed changes
@cchaos cchaos requested a review from elizabetdev December 1, 2021 17:21
@cee-chen
Add new useUpdateEffect hook
413e013 
- 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)
@cee-chen
Add onChange callbacks to gridStyles and rowHeightsOptions
fb5bb63 
 - and fire said callbacks on update whenever user configurations change
@cee-chen
Add documentation example
6b1ba90 
 - with density responsiveness & localStorage
@cee-chen
Add changelog entry
f693f9c
@cee-chen
Copy link
Copy Markdown
Contributor Author

cee-chen commented Dec 1, 2021

OK, I updated this PR to use a new onUpdateEffect helper (fires on update only, not mount). I ended up force pushing because the code was so different, sorry about that - I recommend re-following along by commit. Quick TL;DR of changes:

  • Callbacks are now invoked in an effect and not manually in each state update, which is much more DRY and feels much more React-y
  • Removed passing the onChange function back into the callbacks (inception BWAAM)
  • I removed the misc HTML cleanup commit since I opened up a separate PR for that ([Docs only] Remove HTML sources #5429)
  • I removed the extra callbacks arg I was passing into useDataGridDisplaySelector - I have no idea why I thought that was necessary when I was already passing in gridStyles and rowHeightsOptions 🤦‍♀️
  • Documentation example remained functionally the same with some minor cleanup - I stopped saving the returned objs in state since that didn't work well when combined with [EuiDataGrid] Row height switcher should override rowHeights + add reset button #5428's reset button, and added a console.log for easier debugging

Overall this feels much cleaner than before! I also tested this rebased against #5248 and the onChange callbacks are correctly invoked when the reset button is pressed without any extra code 🎉 The merges will have very minor conflicts as well, so either PR can land first without significant issue

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Dec 1, 2021

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

elizabetdev
elizabetdev approved these changes Dec 2, 2021
View reviewed changes
Copy link
Copy Markdown
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 looks great!

I tested the example code and it works well. The reset button from #5428 now makes even more sense. 👍🏽

Just one small detail. Is it possible to improve the example to also preserve the display settings when the page changes?

Screen.Recording.2021-12-02.at.01.20.PM.mov

@cee-chen
Copy link
Copy Markdown
Contributor Author

cee-chen commented Dec 2, 2021

Oh super interesting, thanks Elizabet, I hadn't tried navigating away. Yes, it's possible, I need to change where I instantiate the localStorage.getItem call into the component itself. I'll do that here shortly.

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Dec 2, 2021

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

chandlerprall
chandlerprall approved these changes Dec 2, 2021
View reviewed changes
Copy link
Copy Markdown
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.

Code changes LGTM, tests are great! Tested locally & in the preview docs.

@cee-chen
Copy link
Copy Markdown
Contributor Author

cee-chen commented Dec 2, 2021

Thanks y'all!

@cee-chen cee-chen merged commit 4559a9e into elastic:feat/datagrid/toolbar-reorg-and-row-height-switcher Dec 2, 2021
@cee-chen cee-chen deleted the datagrid-onchange branch December 2, 2021 17:20
cee-chen pushed a commit that referenced this pull request Dec 7, 2021
@cchaos @chandlerprall
[EuiDataGrid] Feature: toolbar reorganization and row height control (#…
b97f93e 
…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>
@thompsongl thompsongl mentioned this pull request Dec 9, 2021
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@chandlerprall chandlerprall chandlerprall approved these changes

@elizabetdev elizabetdev elizabetdev approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@cee-chen @kibanamachine @chandlerprall @elizabetdev