Allow EuiDataGrid's default column width to update on resize#2991
Conversation
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_2991/ |
thompsongl
left a comment
thompsongl
left a comment
There was a problem hiding this comment.
I'm ok with the approach. It'd be cooler if the observer components were based on shareable hooks and not classes, but I understand modification here
Definitely felt the same as I was putting this together. I'll see what I can do! |
chandlerprall
force-pushed
the
bug/2759-datagrid-responsive-column-widths
branch
from
240b530 to
b3bc695
Compare
| if (nextProps.visibleRowIndex !== this.props.visibleRowIndex) return true; | ||
| if (nextProps.colIndex !== this.props.colIndex) return true; | ||
| if (nextProps.columnId !== this.props.columnId) return true; | ||
| if (nextProps.columnType !== this.props.columnType) return true; |
There was a problem hiding this comment.
If only React had a way to specify dependencies that also included an ESLint rule to tell you when you're missing one. Oh wait ... 😅
Maybe someday this'll be a function component with hooks.
myasonik
left a comment
myasonik
left a comment
There was a problem hiding this comment.
Didn't dig into the code much, just a passing glance, but didn't see anything that would impact the component API or the accessibility so signing off on it.
Will let @thompsongl be the code eyes 🙂
|
@thompsongl refactored everything to be much nicer, updated PR description with more info. |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_2991/ |
src/components/observer/resize_observer/resize_observer.test.tsx
Outdated
Show resolved
Hide resolved
snide
left a comment
snide
left a comment
There was a problem hiding this comment.
I don't have an answer for this and don't consider it a blocker. Just showing it in case anyone has any ideas on how to avoid the "jump". I realize I am a picky designer for even pointing it out. I may or may have recorded a video of this to make sure I wasn't seeing things, which might tell you about how small this issue is 🤣
Ultimately this is a much better tradeoff than what existed before. Tested for functionality and acts how I'd expect.
|
@snide nice catch! Pushed an update to fix that flash |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_2991/ |
snide
left a comment
snide
left a comment
There was a problem hiding this comment.
Yesss! Thank you for the fix. Confirmed locally.
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_2991/ |
5 participants
Summary
Fixes #2759
Problem & reproduction
When originally debugging the issue where datagrid's columns weren't sized correctly on page load, I noticed that at the initial mount, its wrapping flex row wasn't at the full size - only later did it expand to fit available space. I was able to replicate this kind of behaviour in the EUI docs by:
Fix
useResizeObserverhook w/documentation & hookuseResizeObserverto fix the source bug in data gridOther bug
In testing this across browsers, I found that IE11 + Safari (the two remaining with no
ResizeObserversupport) don't trigger the mutation observer fallback, as the content isn't mutated. This should be fixed separately, as it is a bug within theResizeObserverfallback. I've created #3044 to track this.Checklist
- [ ] Check against all themes for compatibility in both light and dark modes- [ ] Props have proper autodocs- [ ] Added documentation examples- [ ] Added or updated jest tests- [ ] Checked for accessibility including keyboard-only and screenreader modes