Skip to content

[EuiDualRange] Add regression test for Kibana use of ref prop#6467

Merged
cee-chen merged 1 commit intoelastic:mainfrom
cee-chen:range-ref
Dec 7, 2022
Merged

[EuiDualRange] Add regression test for Kibana use of ref prop#6467
cee-chen merged 1 commit intoelastic:mainfrom
cee-chen:range-ref

Conversation

@cee-chen
Copy link
Copy Markdown
Contributor

@cee-chen cee-chen commented Dec 7, 2022

Summary

Whether we like it or not, there are currently 2 instances of Kibana using the ref prop on EuiDualRange to call the class onResize method: elastic/kibana@5c575f9. I discovered this because the typing for EuiDualRange's ref prop became extremely annoying once we started wrapping in withEuiTheme.

The ideal solution, converting EuiDualRange from a class component to a function component, unfortunately also takes away Kibana's ability to call onResize. This means if/when we do convert EuiDualRange to a function component, we'll need to deliberately expose onResize via React's useImperativeHandle API.

To ensure we don't forget this, I've added a unit test for calling rangeRef.current.onResize() that will fail if we perform the conversion without adding the ref method.

QA

General checklist

N/A, dev/test only

@cee-chen cee-chen added testing Issues or PRs that only affect tests - will not need changelog entries skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation) labels Dec 7, 2022
@cee-chen cee-chen requested a review from 1Copenut December 7, 2022 00:13
@kibanamachine
Copy link
Copy Markdown

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

Copy link
Copy Markdown
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

👍 LGTM! Your comments and logic make this very easy to follow as a defensive test for future breaking changes.

@cee-chen cee-chen merged commit 1ec1c34 into elastic:main Dec 7, 2022
@cee-chen cee-chen deleted the range-ref branch December 7, 2022 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation) testing Issues or PRs that only affect tests - will not need changelog entries

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants