-
Notifications
You must be signed in to change notification settings - Fork 861
[EuiDataGrid] Allow column.initialWidth+onColumnResize to be used to control column widths
#8082
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| **Bug fixes** | ||
|
|
||
| - Fixed an `EuiDataGrid` regression from [#8015](https://github.com/elastic/eui/pull/8015) to allow `onColumnResize` to control columns' `initialWidth`s again | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -88,7 +88,6 @@ describe('useColumnWidths', () => { | |
| columns: [{ id: 'b', initialWidth: 75 }, { id: 'c' }], | ||
| trailingControlColumns: [{ id: 'd', width: 25 }] as any, | ||
| defaultColumnWidth: 150, | ||
| onColumnResize: jest.fn(), | ||
| }; | ||
|
|
||
| describe('columnWidths', () => { | ||
|
|
@@ -136,16 +135,36 @@ describe('useColumnWidths', () => { | |
| }); | ||
| expect(result.current.columnWidths).toEqual({ b: 150 }); | ||
| }); | ||
|
|
||
| it('does override user resized column widths if `onColumnResize` is passed', () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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!
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should test it, just to be sure. Running through the release process and then finding issues would be annoying 😅
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair! @mgadewoll will you be doing that testing just to check or are you asking me to?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can run it against Kibana. I'll do it a bit later though.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ℹ️ I ran the changes in Kibana and they don't fix the issue. |
||
| const controlledWidthsArgs = { | ||
| ...args, | ||
| onColumnResize: jest.fn(), | ||
| }; | ||
| const { rerender, result } = renderHook(useColumnWidths, { | ||
| initialProps: controlledWidthsArgs, | ||
| }); | ||
|
|
||
| renderHookAct(() => result.current.setColumnWidth('b', 150)); | ||
| rerender({ | ||
| ...controlledWidthsArgs, | ||
| columns: [{ id: 'b', initialWidth: 100 }], | ||
| }); | ||
| expect(result.current.columnWidths).toEqual({ b: 100 }); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe('setColumnWidth', () => { | ||
| it("sets a single column's width in the columnWidths map", () => { | ||
| const { result } = renderHook(() => useColumnWidths(args)); | ||
| const onColumnResize = jest.fn(); | ||
| const { result } = renderHook(() => | ||
| useColumnWidths({ ...args, onColumnResize }) | ||
| ); | ||
|
|
||
| renderHookAct(() => result.current.setColumnWidth('c', 125)); | ||
| expect(result.current.columnWidths).toEqual({ b: 75, c: 125 }); | ||
| expect(args.onColumnResize).toHaveBeenCalledWith({ | ||
| expect(onColumnResize).toHaveBeenCalledWith({ | ||
| columnId: 'c', | ||
| width: 125, | ||
| }); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -79,19 +79,25 @@ export const useColumnWidths = ({ | |
| setColumnWidth: (columnId: string, width: number) => void; | ||
| getColumnWidth: (index: number) => number; | ||
| } => { | ||
| const hasOnColumnResize = !!onColumnResize; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we update the prop docs to make it more clear? Should we also document for consumers that
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I should, I'm being lazy 🫠
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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! 😄
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, let's skip an example for now. I'm honestly not sure if I want people copying it as a pattern 😅 |
||
| // the intent of this prop and how controlled it is, so for now we'll assume | ||
| // that any datagrid passing `onColumnResize` is controlling its column widths | ||
| // and should override any internal column width state set by user resizing | ||
| if (hasOnColumnResize || columnWidths[id] == null) { | ||
| columnWidths[id] = initialWidth!; | ||
| } | ||
| }); | ||
| return columnWidths; | ||
| }, | ||
| [columns] | ||
| [columns, hasOnColumnResize] | ||
| ); | ||
|
|
||
| // Passes initializer function for performance, so computing only runs once on init | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.