-
Notifications
You must be signed in to change notification settings - Fork 839
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 onChange
callbacks for display selector changes
#5424
[EuiDataGrid] Add onChange
callbacks for display selector changes
#5424
Conversation
Preview documentation changes for this PR: https://eui.elastic.co/pr_5424/ |
- 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)
- and fire said callbacks on update whenever user configurations change
- with density responsiveness & localStorage
c2687f9
to
f693f9c
Compare
OK, I updated this PR to use a new
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 |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5424/ |
There was a problem hiding this 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
Oh super interesting, thanks Elizabet, I hadn't tried navigating away. Yes, it's possible, I need to change where I instantiate the |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5424/ |
There was a problem hiding this 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.
Thanks y'all! |
4559a9e
into
elastic:feat/datagrid/toolbar-reorg-and-row-height-switcher
…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>
Summary
Per feedback from Caroline and Vlad, we should expose internal
gridStyle
androwHeightsOptions
state in anonChange
callback:After discussion with @chandlerprall, the API we settled on was adding these callbacks in
gridStyle.onChange
androwHeightsOptions.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