Skip to content

Conversation

@cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Feb 1, 2022

Summary

closes #5587

TL;DR: On sort, in memory tables' onTableChange return the correct sort.field value as column.field, but on pagination, it incorrectly returns sort.field as column.name.

Before

before

Jest test failure comparison:

After

after

Checklist

- [ ] Check against all themes for compatibility in both light and dark modes
- [ ] Checked in mobile
- [ ] Checked in Chrome, Safari, Edge, and Firefox
- [ ] Props have proper autodocs and playground toggles
- [ ] Added documentation
- [ ] Checked Code Sandbox works for any docs examples

- [ ] Checked for breaking changes and labeled appropriately
- [ ] Checked for accessibility including keyboard-only and screenreader modes

  • A changelog entry exists and is marked appropriately
  • Revert [REVERT] demo commit

@cee-chen cee-chen requested a review from thompsongl February 1, 2022 19:47
@cee-chen cee-chen force-pushed the in-memory-table/5587 branch from f625c78 to e1cee95 Compare February 1, 2022 19:50
Comment on lines 408 to 411
let sortColumn = findColumnByProp(columns, 'field', sortName as string);
if (sortColumn == null) {
sortColumn = findColumnByProp(columns, 'name', sortName as string);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied this handy 'findColumnBy' logic up above in getInitialSorting:

// sortable could be a column's `field` or its `name`
// for backwards compatibility `field` must be checked first
let sortColumn = findColumnByProp(columns, 'field', sortable);
if (sortColumn == null) {
sortColumn = findColumnByProp(columns, 'name', sortable);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Want to make this a function that's reused both places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooo, you know it!! 16dc234

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

🙏 Thanks for the new test and additional comments!

One suggestion but the rest LGTM

Don't forget your REVERT commit 🎗️

Comment on lines 408 to 411
let sortColumn = findColumnByProp(columns, 'field', sortName as string);
if (sortColumn == null) {
sortColumn = findColumnByProp(columns, 'name', sortName as string);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Want to make this a function that's reused both places?

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5588/

+ fix value typing to accept names which can be ReactNodes
@cee-chen cee-chen enabled auto-merge (squash) February 1, 2022 20:38
@cee-chen cee-chen disabled auto-merge February 1, 2022 21:00
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5588/

@cee-chen cee-chen enabled auto-merge (squash) February 1, 2022 23:11
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5588/

@cee-chen cee-chen merged commit f83d168 into elastic:main Feb 2, 2022
@cee-chen cee-chen deleted the in-memory-table/5587 branch February 2, 2022 00:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[EuiInMemoryTable] onTableChange returns inconsistent sort.field property

3 participants