Skip to content

Comments

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

Merged
cee-chen merged 5 commits intoelastic:masterfrom
cee-chen:datagrid-cellprops-reorder
Aug 24, 2021
Merged

[EuiDataGrid] Fix cell props not clearing state on column reorder#5068
cee-chen merged 5 commits intoelastic:masterfrom
cee-chen:datagrid-cellprops-reorder

Conversation

@cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Aug 23, 2021 •
edited
Loading

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

before

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 the 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

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

cee-chen commented Aug 23, 2021 •
edited
Loading

@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
cee-chen commented Aug 23, 2021
View reviewed changes
src/components/datagrid/body/data_grid_cell.tsx
Comment on lines +254 to +258
componentDidUpdate(prevProps: EuiDataGridCellProps) {
if (this.props.columnId !== prevProps.columnId) {
this.setCellProps({});
}
}
Copy link
Contributor Author

@cee-chen cee-chen Aug 23, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@thompsongl thompsongl Aug 24, 2021

Choose a reason for hiding this comment

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

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

@cee-chen cee-chen mentioned this pull request Aug 23, 2021
Closed
@kibanamachine
Copy link

kibanamachine commented Aug 23, 2021

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

cee-chen
cee-chen commented Aug 23, 2021
View reviewed changes
src/components/datagrid/data_grid.test.tsx
]);
});

it('resets cell props on column reorder', () => {
Copy link
Contributor Author

@cee-chen cee-chen Aug 23, 2021

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

@thompsongl thompsongl Aug 24, 2021

Choose a reason for hiding this comment

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

I'm in favor of verbose testing 👍

@kibanamachine
Copy link

kibanamachine commented Aug 23, 2021

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

thompsongl
thompsongl approved these changes Aug 24, 2021
View reviewed changes
src/components/datagrid/data_grid.test.tsx
]);
});

it('resets cell props on column reorder', () => {
Copy link
Contributor

@thompsongl thompsongl Aug 24, 2021

Choose a reason for hiding this comment

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

I'm in favor of verbose testing 👍

src/components/datagrid/body/data_grid_cell.tsx
Comment on lines +254 to +258
componentDidUpdate(prevProps: EuiDataGridCellProps) {
if (this.props.columnId !== prevProps.columnId) {
this.setCellProps({});
}
}
Copy link
Contributor

@thompsongl thompsongl Aug 24, 2021

Choose a reason for hiding this comment

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

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

@kibanamachine
Copy link

kibanamachine commented Aug 24, 2021

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

@cee-chen
Copy link
Contributor Author

cee-chen commented Aug 24, 2021

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!

@cee-chen cee-chen merged commit 6c48745 into elastic:master Aug 24, 2021
@cee-chen cee-chen deleted the datagrid-cellprops-reorder branch August 24, 2021 16:34
This was referenced Aug 24, 2021
Closed
Merged
cee-chen added a commit to cee-chen/kibana that referenced this pull request Aug 25, 2021
@cee-chen
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
@cee-chen
Revert "Remove hook cleanup now that elastic/eui#5068 is merged"
5861c94 
This reverts commit e40ebfa.
cee-chen pushed a commit to elastic/kibana that referenced this pull request Sep 1, 2021
@kibanamachine
Upgrade EUI to v37.3.1 (#109926)
47b86e1 
* 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>
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Sep 1, 2021
@kibanamachine
Upgrade EUI to v37.3.1 (elastic#109926)
8d6268a 
* 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>
kibanamachine added a commit to elastic/kibana that referenced this pull request Sep 1, 2021
@kibanamachine
Upgrade EUI to v37.3.1 (#109926) (#110833)
8602f4f 
* 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>
This was referenced Feb 28, 2022
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@thompsongl thompsongl thompsongl approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[EuiDataGrid] Cells styling stick to cell's position when adding new columns [EuiDataGrid] Cell styling retains when column position is changed

3 participants

@cee-chen @kibanamachine @thompsongl