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.

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

[EuiInMemoryTable] onTableChange returns inconsistent sort.field property #5587

Closed
qn895 opened this issue Feb 1, 2022 · 3 comments · Fixed by #5588
Closed

[EuiInMemoryTable] onTableChange returns inconsistent sort.field property #5587

qn895 opened this issue Feb 1, 2022 · 3 comments · Fixed by #5588
Assignees
Labels

Comments

@qn895
Copy link
Member

qn895 commented Feb 1, 2022

If we change the sorting of an InMemoryTable, the sort.field is an id. But if I change pagination setting, the resulting sort.field is now using the column’s name instead. I can replicate this with sandbox on EUI 46.1.0. Here's link to sandbox to reproduce.

Screen.Recording.2022-01-31.at.14.51.46.mov
@qn895 qn895 added the bug label Feb 1, 2022
@thompsongl
Copy link
Contributor

thompsongl commented Feb 1, 2022

This looks like it was intentional at some point:

// EuiBasicTable returns the column's `field` if it exists instead of `name`,
// map back to `name` if this is the case
for (let i = 0; i < this.props.columns.length; i++) {
const column = this.props.columns[i];
if (
'field' in column &&
(column as EuiTableFieldDataColumnType<T>).field === sortName
) {
sortName = column.name as keyof T;
break;
}
}

It doesn't make any sense to me to return name if we have field, though, unless it was preventing a breaking change. Simply removing the code above resolves the discrepancy, but surely (re)introduces some other edge-case bugs.

@chandlerprall do you have any context here? This code is a couple years old.

@cee-chen cee-chen self-assigned this Feb 1, 2022
@cee-chen
Copy link
Contributor

cee-chen commented Feb 1, 2022

It appears to be an intentional architectural decision to store sortName as the display name / ReactNode internally and not the sortField ID, but the fact that pagination returns column.name and not column.field is a bug/oversight - reportedSortName up above (which is what gets returned to the onTableChange callback) needs to be overridden and return column.field if it exists. I poked at this and have a fix that I believe should work, I'll open a PR shortly.

@chandlerprall
Copy link
Contributor

@chandlerprall do you have any context here? This code is a couple years old.

Looks like Constance found the right thread to follow, but context for that snippet is that it enabled sorting of computed columns, introduced in PR #2044 to solve #2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants