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

Perf: EuiDataGrid don't recreate columns on every change #2676

Merged
merged 4 commits into from
Dec 17, 2019
Merged

Perf: EuiDataGrid don't recreate columns on every change #2676

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bddba43
perf: dont recreate columns on every change
mbondyra Dec 13, 2019
4ce3bc5
Update CHANGELOG.md
mbondyra Dec 17, 2019
82f078a
revert: the big table size
mbondyra Dec 17, 2019
9aeed67
Merge branch 'master' into perf-data-grid-take-2
mbondyra Dec 17, 2019
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
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
## [`master`](https://github.com/elastic/eui/tree/master)

No public interface changes since `17.2.1`.
- Improved `EuiDataGrid` update performance ([#2676](https://github.com/elastic/eui/pull/2676))

## [`17.2.1`](https://github.com/elastic/eui/tree/v17.2.1)

Expand Down
4 changes: 2 additions & 2 deletions src-docs/src/views/datagrid/datagrid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ const columns = [

const raw_data = [];

for (let i = 1; i < 100; i++) {
for (let i = 1; i < 500; i++) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do accept that kind of size in Kibana so maybe it's worth to add it here?

Copy link
Contributor

Choose a reason for hiding this comment

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

You could always make this dynamic through state, and let the user define it in the docs. The reason I kept it low was that I cared more about the docs page being snappy, then checking the performance of the grid itself. But now that you've made these changes it's a less a concern.

Copy link
Contributor Author

@mbondyra mbondyra Dec 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think you're right! The initial load time doesn't change so it's worth to lower it. Thank you for this comment!

raw_data.push({
name: fake('{{name.lastName}}, {{name.firstName}} {{name.suffix}}'),
email: <EuiLink href="">{fake('{{internet.email}}')}</EuiLink>,
Expand Down Expand Up @@ -141,7 +141,7 @@ export default () => {
sorting={{ columns: sortingColumns, onSort }}
pagination={{
...pagination,
pageSizeOptions: [10, 50, 100],
pageSizeOptions: [10, 50, 100, 500],
onChangeItemsPerPage: onChangeItemsPerPage,
onChangePage: onChangePage,
}}
Expand Down
20 changes: 14 additions & 6 deletions src/components/datagrid/column_selector.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import React, {
Fragment,
useState,
useMemo,
ReactChild,
ReactElement,
ChangeEvent,
Expand Down Expand Up @@ -199,11 +200,18 @@ export const useColumnSelector = (
</EuiPopover>
);

const orderedVisibleColumns = visibleColumns
.map<EuiDataGridColumn>(
columnId =>
availableColumns.find(({ id }) => id === columnId) as EuiDataGridColumn // cast to avoid `undefined`, it filters those out next
)
.filter(column => column != null);
const orderedVisibleColumns = useMemo(
() =>
visibleColumns
.map<EuiDataGridColumn>(
columnId =>
availableColumns.find(
({ id }) => id === columnId
) as EuiDataGridColumn // cast to avoid `undefined`, it filters those out next
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not filter first?

Copy link
Contributor Author

@mbondyra mbondyra Dec 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think (although I am not sure, I just wrapped it with useMemo but that's how I understand the comment) that the find returns nulls in case it won't find the id === columnId and after that we're getting rid of the nulls with filter 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, this looks like whitespace-only changes with the additional useMemo wrapping. I set originally set it up to filter second. No real reason, didn't occur to me to filter first 🤷‍♀

)
.filter(column => column != null),
[availableColumns, visibleColumns]
);

return [columnSelector, orderedVisibleColumns];
};