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

[EuiInMemoryTable] Error appears in console related to prop sorting.sort.direction #4138

Merged
merged 5 commits into from
Nov 4, 2020
Merged

[EuiInMemoryTable] Error appears in console related to prop sorting.sort.direction #4138

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
62c45ee
[EuiInMemoryTable] Error appears in console related to prop `sorting.…
DianaDerevyankina Oct 15, 2020
79349c8
Add descrition to CHANGELOG
DianaDerevyankina Oct 15, 2020
33ff340
Merge branch 'master' into Diana/4137
DianaDerevyankina Nov 3, 2020
aa3b4af
Replace columnField with type guard for field and create a unit test …
DianaDerevyankina Nov 3, 2020
809da82
Merge branch 'master' into Diana/4137
chandlerprall Nov 4, 2020
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

- Fixed custom color render of `EuiIcon` app (two-tone) icons ([#4104](https://github.com/elastic/eui/pull/4104))
- Changed `iconType` prop to be `required` in `EuiButtonIcon` ([#4106](https://github.com/elastic/eui/pull/4106))
- Fixed a condition in `EuiInMemoryTable` to avoid mistaken assignment of `sortName` ([#4138](https://github.com/elastic/eui/pull/4138))

## [`29.4.0`](https://github.com/elastic/eui/tree/v29.4.0)

Expand Down
3 changes: 2 additions & 1 deletion src/components/basic_table/in_memory_table.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -410,7 +410,8 @@ export class EuiInMemoryTable<T> extends Component<
// 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 ((column as EuiTableFieldDataColumnType<T>).field === sortName) {
const columnField = (column as EuiTableFieldDataColumnType<T>).field;
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the PR submission! This does solve the problem, but I'm a little worried about future changes to this code accidentally undoing the fix. Please include two modifications to help future-proof this code:

  1. Instead of always pulling columnField, please use a type guard instead. A quick test if the column contains the field value would suffice to determine if a field is EuiTableFieldDataColumnType. Using a type guard here helps the code self-document what it is doing.

  2. Add a unit test to in_memory_table.test.tsx under the behavior section. There's an existing pagination test that you can copy the format of, but if you're unsure how to set it up please ask! The test need only change the number of displayed rows, we have a utility that automatically fails a test if a proptype check fails.

if (columnField && columnField === sortName) {
sortName = column.name as keyof T;
break;
}
Expand Down