[EuiDataGrid] Allow column.initialWidth+onColumnResize to be used to control column widths
#8082
Conversation
| @@ -79,19 +79,25 @@ export const useColumnWidths = ({ | |||
| setColumnWidth: (columnId: string, width: number) => void; | |||
| getColumnWidth: (index: number) => number; | |||
| } => { | |||
| const hasOnColumnResize = !!onColumnResize; | |||
There was a problem hiding this comment.
So simple, but I like it 🎉
| const getInitialWidths = useCallback( | ||
| (prevColumnWidths?: EuiDataGridColumnWidths) => { | ||
| const columnWidths = { ...prevColumnWidths }; | ||
| columns | ||
| .filter(doesColumnHaveAnInitialWidth) | ||
| .forEach(({ id, initialWidth }) => { | ||
| if (columnWidths[id] == null) { | ||
| // Several Kibana datagrids are using `onColumnResize` and `column.initialWidth` | ||
| // to fully control column widths. Sadly, we didn't do a good job documenting |
There was a problem hiding this comment.
Should we update the prop docs to make it more clear? Should we also document for consumers that onColumnResize is a flag for this behavior?
There was a problem hiding this comment.
Yeah, I should, I'm being lazy 🫠
There was a problem hiding this comment.
ce84874 - do you think this is sufficient or should we also consider adding a full-fledged example of fully controlled column widths? Maybe under the Advanced section?
There was a problem hiding this comment.
The description is great! Thanks.
I'm considering the example, but I'm torn if it's "standard" behavior and doesn't need documenting (or do we even want it?) and if it's better to just document everything, because you know ... people! 😄
There was a problem hiding this comment.
Yeah, let's skip an example for now. I'm honestly not sure if I want people copying it as a pattern 😅
| @@ -136,16 +135,36 @@ describe('useColumnWidths', () => { | |||
| }); | |||
| expect(result.current.columnWidths).toEqual({ b: 150 }); | |||
| }); | |||
|
|
|||
| it('does override user resized column widths if `onColumnResize` is passed', () => { | |||
There was a problem hiding this comment.
This should be enough to be confident, but just to be super sure: Should we run a test package on Kibana to verify it fixed the tests? (or maybe you already did)
There was a problem hiding this comment.
Up to you since you're running this week's release/upgrade - I could go either way, but I also probably yolo too much 🤪 Going through the prerelease/release candidate process to test Kibana CI here could definitely be helpful as practice!
There was a problem hiding this comment.
I think we should test it, just to be sure. Running through the release process and then finding issues would be annoying 😅
There was a problem hiding this comment.
Fair! @mgadewoll will you be doing that testing just to check or are you asking me to?
There was a problem hiding this comment.
I can run it against Kibana. I'll do it a bit later though.
There was a problem hiding this comment.
ℹ️ I ran the changes in Kibana and they don't fix the issue.
We decided to revert the changes for now instead (PR) and address them separately as they were not specific to the EuiDataGrid draggable column update.
|
Preview staging links for this PR:
|
💚 Build Succeeded
History
|
|
Closing this PR in favor of a full revert: #8086 |
Summary
This partially reverts the column width cache/state storage we added in #8015 (comment) and #8015 (comment).
It turns out Kibana is using
onColumnResizeway more than I had realized, andinitialWidth(which I had assumed to be mostly used for uncontrolled usage, as its naming is similar todefaultValue) is being used fairly heavily in combination withonColumnResizeto control column widths on update.This PR allows for that controlled usage while still maintaining uncontrolled in-memory user resized column widths for datagrids that do not have a
onColumnResizecallback passed.QA
General checklist
onColumnResizethoroughly anywhere 🫠- [ ] Added documentation- [ ] Props have proper autodocs (using@defaultif default values are missing) and playground togglesand cypress tests- [ ] Updated visual regression tests- [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)