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.

Sign up for GitHub

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

Jump to bottom

[EuiDataGrid] Allow column.initialWidth+onColumnResize to be used to control column widths #8082

Closed
wants to merge 3 commits into from
Closed

[EuiDataGrid] Allow column.initialWidth+onColumnResize to be used to control column widths #8082

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cc2bf82
Sorta revert column widths cache/state storage added in draggable col…
cee-chen Oct 16, 2024
29b913a
changelog
cee-chen Oct 16, 2024
ce84874
prop documentation of behavior
cee-chen Oct 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/eui/changelogs/upcoming/8082.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
**Bug fixes**
mgadewoll marked this conversation as resolved.
Show resolved Hide resolved

- Fixed an `EuiDataGrid` regression from [#8015](https://github.com/elastic/eui/pull/8015) to allow `onColumnResize` to control columns' `initialWidth`s again
25 changes: 22 additions & 3 deletions packages/eui/src/components/datagrid/utils/col_widths.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -136,16 +135,36 @@ describe('useColumnWidths', () => {
});
expect(result.current.columnWidths).toEqual({ b: 150 });
});

it('does override user resized column widths if `onColumnResize` is passed', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The 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)

Copy link
Member Author

Choose a reason for hiding this comment

The 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!

Copy link
Contributor

Choose a reason for hiding this comment

The 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 😅

Copy link
Member Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.
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.

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,
});
Expand Down
10 changes: 8 additions & 2 deletions packages/eui/src/components/datagrid/utils/col_widths.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,19 +79,25 @@ export const useColumnWidths = ({
setColumnWidth: (columnId: string, width: number) => void;
getColumnWidth: (index: number) => number;
} => {
const hasOnColumnResize = !!onColumnResize;
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The 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 onColumnResize is a flag for this behavior?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I should, I'm being lazy 🫠

Copy link
Member Author

Choose a reason for hiding this comment

The 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 Advanced section?

Copy link
Contributor

Choose a reason for hiding this comment

The 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! 😄

Copy link
Member Author

Choose a reason for hiding this comment

The 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
Expand Down
Loading