-
Notifications
You must be signed in to change notification settings - Fork 837
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] Fix onTableChange
returning the sort field's name
instead of field
key
#5588
Conversation
- Should currently fail with field returns as `Name` instead of `name`
f625c78
to
e1cee95
Compare
let sortColumn = findColumnByProp(columns, 'field', sortName as string); | ||
if (sortColumn == null) { | ||
sortColumn = findColumnByProp(columns, 'name', sortName as string); | ||
} |
There was a problem hiding this comment.
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
:
eui/src/components/basic_table/in_memory_table.tsx
Lines 221 to 226 in e1cee95
// 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); | |
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this 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 🎗️
let sortColumn = findColumnByProp(columns, 'field', sortName as string); | ||
if (sortColumn == null) { | ||
sortColumn = findColumnByProp(columns, 'name', sortName as string); | ||
} |
There was a problem hiding this comment.
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?
Preview documentation changes for this PR: https://eui.elastic.co/pr_5588/ |
+ fix value typing to accept names which can be ReactNodes
This reverts commit 3c1e939.
Preview documentation changes for this PR: https://eui.elastic.co/pr_5588/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5588/ |
Summary
closes #5587
TL;DR: On sort, in memory tables'
onTableChange
return the correctsort.field
value ascolumn.field
, but on pagination, it incorrectly returnssort.field
ascolumn.name
.Before
Jest test failure comparison:
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