[EuiDataGrid] Fix cell props not clearing state on column reorder #5068

cee-chen · 2021-08-23T20:28:33Z

Summary

closes #4599, and I believe should also close #4665

The before/after screenshots depict the issue quite clearly, so I'll let a picture speak a thousand words for me 😄

Before

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 the any docs examples~~

Added or updated jest tests

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

A changelog entry exists and is marked appropriately

cee-chen · 2021-08-23T20:31:25Z

@chandlerprall @thompsongl - I'm not super clear on how jest/unit tests are currently being handled for EuiDataGrid. Currently I'm not seeing any .test.tsx files for child EuiDataGrid components, so I don't think there's anything for me to write in here unless you'd like me to start one?

Alternatively, I can make a note/self-todo to write a Cypress test for this specifically once the current Cypress tests land - thoughts?

EDIT: Unit test added, see #5068 (review)

cee-chen · 2021-08-23T20:33:16Z

src/components/datagrid/body/data_grid_cell.tsx

+  componentDidUpdate(prevProps: EuiDataGridCellProps) {
+    if (this.props.columnId !== prevProps.columnId) {
+      this.setCellProps({});
+    }
+  }


Let me know if you have a preferred/cleaner approach of solving this issue! I'm not in love with this or anything, although it seemed like the most straightforward solution - but I could be definitely missing something or an edge case.

This makes sense to me. @chandlerprall can chime in if he has other thoughts, though.

kibanamachine · 2021-08-23T20:54:26Z

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

cee-chen · 2021-08-23T21:48:07Z

src/components/datagrid/data_grid.test.tsx

@@ -1467,6 +1467,48 @@ describe('EuiDataGrid', () => {
        ['1-ColumnB', '1-ColumnA'],
      ]);
    });
+
+    it('resets cell props on column reorder', () => {


@chandlerprall requested a unit test for this in Slack! This is mostly a direct copy/paste of the above column order test in terms of setup (but with 1 less row and setCellProps in renderCellValue). I'm not sure if there's any DRYing out I should be doing between the 2 reorder unit tests - happy to do so if requested.

I also ran this test on master and can confirm it fails there but passes on this branch.

I'm in favor of verbose testing 👍

kibanamachine · 2021-08-23T22:09:14Z

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

thompsongl · 2021-08-24T14:10:24Z

src/components/datagrid/data_grid.test.tsx

@@ -1467,6 +1467,48 @@ describe('EuiDataGrid', () => {
        ['1-ColumnB', '1-ColumnA'],
      ]);
    });
+
+    it('resets cell props on column reorder', () => {


I'm in favor of verbose testing 👍

thompsongl · 2021-08-24T14:13:33Z

src/components/datagrid/body/data_grid_cell.tsx

+  componentDidUpdate(prevProps: EuiDataGridCellProps) {
+    if (this.props.columnId !== prevProps.columnId) {
+      this.setCellProps({});
+    }
+  }


This makes sense to me. @chandlerprall can chime in if he has other thoughts, though.

kibanamachine · 2021-08-24T15:27:09Z

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

cee-chen · 2021-08-24T16:32:35Z

Going to go ahead and merge to get into the release today :) I'm happy to revert or open a follow-up PR if any changes are requested!

This reverts commit e40ebfa.

* Upgrade EUI to v37.3.1 * Update i18n token mappings * Skip i18n_eui_mapping defString checks for functions * Update snapshots * Update failing Security tests with extra nodes * Remove hook cleanup now that elastic/eui#5068 is merged * [i18n PR feedback] Prefer specific token skipping over all functions skipping * Revert "Remove hook cleanup now that elastic/eui#5068 is merged" This reverts commit e40ebfa. Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* Upgrade EUI to v37.3.1 * Update i18n token mappings * Skip i18n_eui_mapping defString checks for functions * Update snapshots * Update failing Security tests with extra nodes * Remove hook cleanup now that elastic/eui#5068 is merged * [i18n PR feedback] Prefer specific token skipping over all functions skipping * Revert "Remove hook cleanup now that elastic/eui#5068 is merged" This reverts commit e40ebfa. Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Constance <constancecchen@users.noreply.github.com>

cee-chen added 2 commits August 23, 2021 13:15

Fix cell props not clearing state on column reorder

262ed15

Add changelog entry

69f815a

cee-chen force-pushed the datagrid-cellprops-reorder branch from f824861 to 69f815a Compare August 23, 2021 20:31

cee-chen commented Aug 23, 2021

View reviewed changes

cee-chen mentioned this pull request Aug 23, 2021

[EuiDataGrid] Cells styling stick to cell's position when adding new columns #4665

Closed

cee-chen added 2 commits August 23, 2021 14:43

Add data_grid unit test

605850b

[misc] Fix typo in test name

318dcd5

cee-chen commented Aug 23, 2021

View reviewed changes

thompsongl approved these changes Aug 24, 2021

View reviewed changes

Merge branch 'master' into datagrid-cellprops-reorder

dddeb3b

cee-chen merged commit 6c48745 into elastic:master Aug 24, 2021

cee-chen deleted the datagrid-cellprops-reorder branch August 24, 2021 16:34

This was referenced Aug 24, 2021

[EuiDataGrid] Fixed: Cell styling retains when column position is changed #4601

Closed

Upgrade EUI to v37.3.1 elastic/kibana#109926

Merged

cee-chen added a commit to cee-chen/kibana that referenced this pull request Aug 25, 2021

Remove hook cleanup now that elastic/eui#5068 is merged

e40ebfa

cee-chen added a commit to cee-chen/kibana that referenced this pull request Aug 31, 2021

Revert "Remove hook cleanup now that elastic/eui#5068 is merged"

5861c94

This reverts commit e40ebfa.

This was referenced Feb 28, 2022

[EuiDataGrid] Cell style sticks to position when sorting #5583

Closed

[EuiDataGrid] Fix cell props not resetting on column sort #5665

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiDataGrid] Fix cell props not clearing state on column reorder #5068

[EuiDataGrid] Fix cell props not clearing state on column reorder #5068

cee-chen commented Aug 23, 2021 •

edited

Loading

cee-chen commented Aug 23, 2021 •

edited

Loading

cee-chen Aug 23, 2021

thompsongl Aug 24, 2021

kibanamachine commented Aug 23, 2021

cee-chen Aug 23, 2021

thompsongl Aug 24, 2021

kibanamachine commented Aug 23, 2021

thompsongl Aug 24, 2021

thompsongl Aug 24, 2021

kibanamachine commented Aug 24, 2021

cee-chen commented Aug 24, 2021

[EuiDataGrid] Fix cell props not clearing state on column reorder #5068

[EuiDataGrid] Fix cell props not clearing state on column reorder #5068

Conversation

cee-chen commented Aug 23, 2021 • edited Loading

Summary

Before

After

Checklist

cee-chen commented Aug 23, 2021 • edited Loading

cee-chen Aug 23, 2021

Choose a reason for hiding this comment

thompsongl Aug 24, 2021

Choose a reason for hiding this comment

kibanamachine commented Aug 23, 2021

cee-chen Aug 23, 2021

Choose a reason for hiding this comment

thompsongl Aug 24, 2021

Choose a reason for hiding this comment

kibanamachine commented Aug 23, 2021

thompsongl Aug 24, 2021

Choose a reason for hiding this comment

thompsongl Aug 24, 2021

Choose a reason for hiding this comment

kibanamachine commented Aug 24, 2021

cee-chen commented Aug 24, 2021

cee-chen commented Aug 23, 2021 •

edited

Loading

cee-chen commented Aug 23, 2021 •

edited

Loading