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] Update display selector UI when gridStyle and rowHeightsOptions props change #5526

Merged
merged 10 commits into from
Jan 12, 2022

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Jan 10, 2022

Summary

Before

before.mp4

After

after.mp4

Caveat

Note that this fix only applies to prop changes that occur before the user interacts with the display selector. Once the user has chosen their own settings, those user settings will always override any passed props, regardless of whether the props update later. See the below example:

caveat

QA

  • Go to https://eui.elastic.co/pr_5526/#/tabular-content/data-grid-styling-and-control (first demo)
  • Open the display selector, confirm it's set to Compact and Single
  • Click the gridStyle button and set fontSize and cellPadding both to large
  • Open the display selector again and confirm density is now set to Expanded
  • Click the Set row height to auto test button
  • Open the display selector again and confirm row height is now set to Auto

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

- [ ] Checked for breaking changes and labeled appropriately
- [ ] Checked for accessibility including keyboard-only and screenreader modes

  • A changelog entry exists and is marked appropriately
  • Revert docs commit

…ptions are passed by consumers

- Previously useState() was caching initial styles/row heights so that developer prop changes (if not overridden by user settings) weren't reflected in the UI

- This fix:
  - Adds `useEffect`s that fires whenever the merged consumer/user states change (so will automatically always set UI state based on most recent configurations)
   - Refactors out the need for the `intial*` vars
  - Refactors out the need for manual UI state setting within UI callbacks
  - Cleans up resetToInitialState callback to only have to set user configs back to empty objs
- now that we're no longer storing initial UI state

- NB: this new `showResetButton` state is slightly less granular than the previous logic, because it doesn't check if the display state was changed back to the original initial state and only checks for a coarse 'did the user interact with the controls', but IMO this is an acceptable tradeoff and adds minimal friction in exchange for easier developer maintainability
- our merged configs are guaranteed to always be at least an empty object, so we can now remove some undefined checks
- unfortunately `shallow` in Enzyme does not invoke useEffect, so the lineCount UI no longer appears in the changed test. I modified the test to simply no longer have that call (as it was tangential to the main test case)
@kibanamachine
Copy link

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

Copy link
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.

Left 2 thoughts, one wouldn't be actionable until some other PRs merge as well.

cee-chen and others added 2 commits January 11, 2022 13:09
+ perf optimization - wrap grid setup in useMemo/useCallbacks to prevent extra unnecessary calls
@kibanamachine
Copy link

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

1 similar comment
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@cee-chen cee-chen enabled auto-merge (squash) January 12, 2022 19:58
@kibanamachine
Copy link

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

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.

[EuiDataGrid] Row height options props update doesn't work
3 participants