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

Jump to bottom

[Canvas] Refactor Canvas to no longer use componentWillReceiveProps #52129

Merged
merged 8 commits into from
Dec 30, 2019
Merged

[Canvas] Refactor Canvas to no longer use componentWillReceiveProps #52129

merged 8 commits into from
Dec 30, 2019

Conversation

poffdeluxe
Copy link
Contributor

@poffdeluxe poffdeluxe commented Dec 3, 2019 •
edited
Loading

Summary

Removing componentWillReceiveProps and replacing it with componentDidUpdate in most places.

For the Expression component, we're using a React key to force the component to reset state (as recommended by the React folks here: https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html#recommendation-fully-uncontrolled-component-with-a-key)

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@poffdeluxe poffdeluxe added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas loe:small Small Level of Effort release_note:skip Skip the PR/issue when compiling release notes impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. labels Dec 3, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-canvas (Team:Canvas)

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@poffdeluxe poffdeluxe marked this pull request as ready for review December 12, 2019 21:29
@poffdeluxe poffdeluxe requested a review from a team as a code owner December 12, 2019 21:29
@poffdeluxe
Copy link
Contributor Author

@elasticmachine merge upstream

crob611
crob611 approved these changes Dec 20, 2019
View reviewed changes
Copy link
Contributor

@crob611 crob611 left a comment

Choose a reason for hiding this comment

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

Changes look good. Tested all of the scenarios I could think of involving these and didn't see anything out of the ordinary. 👍

@poffdeluxe
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@elastic elastic deleted a comment from elasticmachine Dec 30, 2019
@poffdeluxe poffdeluxe merged commit b8046c7 into elastic:master Dec 30, 2019
@poffdeluxe poffdeluxe deleted the refactor-will-receive-props branch December 30, 2019 15:20
poffdeluxe added a commit to poffdeluxe/kibana that referenced this pull request Dec 30, 2019
@poffdeluxe @elasticmachine
[Canvas] Refactor Canvas to no longer use componentWillReceiveProps (e…
da1bd1c 
…lastic#52129)

* Removing componentWillReceiveProps from time filter

* Changing expression form to componentDidUpdate

* Updating expression to be key-driven updates and arg_types to use compomentDidUpdate

* temporary

* Revert "temporary"

This reverts commit 255525d.

* typo fix

Co-authored-by: Elastic Machine <[email protected]>
@poffdeluxe poffdeluxe mentioned this pull request Dec 30, 2019
Merged
poffdeluxe added a commit that referenced this pull request Dec 30, 2019
@poffdeluxe @elasticmachine
[Canvas] Refactor Canvas to no longer use componentWillReceiveProps (#…
a1c3186 
…52129) (#53843)

* Removing componentWillReceiveProps from time filter

* Changing expression form to componentDidUpdate

* Updating expression to be key-driven updates and arg_types to use compomentDidUpdate

* temporary

* Revert "temporary"

This reverts commit 255525d.

* typo fix

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 2, 2020
@gmmorris
Merge branch 'master' into alerting/zombie-tasks
6223180 
* master:
  Bump year in NOTICE.txt
  Add kibanamachine support to Github PR comments (elastic#53852)
  Add tests to ensure AAD isn't broken after performing a change on an alert / action (elastic#53333)
  Skip failing test suite
  [Vega] Sample [Flights] Airport Connections (Hover Over Airport) visualization not working (elastic#53799)
  Do not remount applications between page navigations (elastic#53851)
  [Canvas] Refactor Canvas to no longer use componentWillReceiveProps (elastic#52129)
  [Canvas] Migrate usage collector to NP plugin (elastic#53303)
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 2, 2020
@gmmorris
Merge branch 'master' into alerting/created_at-and-updated_at
fe1902e 
* master:
  Bump year in NOTICE.txt
  Add kibanamachine support to Github PR comments (elastic#53852)
  Add tests to ensure AAD isn't broken after performing a change on an alert / action (elastic#53333)
  Skip failing test suite
  [Vega] Sample [Flights] Airport Connections (Hover Over Airport) visualization not working (elastic#53799)
  Do not remount applications between page navigations (elastic#53851)
  [Canvas] Refactor Canvas to no longer use componentWillReceiveProps (elastic#52129)
  [Canvas] Migrate usage collector to NP plugin (elastic#53303)
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 2, 2020
@gmmorris
Merge branch 'master' into alerting/client-types
fa3e74f 
* master:
  Bump year in NOTICE.txt
  Add kibanamachine support to Github PR comments (elastic#53852)
  Add tests to ensure AAD isn't broken after performing a change on an alert / action (elastic#53333)
  Skip failing test suite
  [Vega] Sample [Flights] Airport Connections (Hover Over Airport) visualization not working (elastic#53799)
  Do not remount applications between page navigations (elastic#53851)
  [Canvas] Refactor Canvas to no longer use componentWillReceiveProps (elastic#52129)
  [Canvas] Migrate usage collector to NP plugin (elastic#53303)
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 2, 2020
@gmmorris
Merge branch 'master' into task-manager/schema-config
4246b94 
* master:
  Fix suggested value for time_zone in range query (elastic#53841)
  Clean up generic hooks, use react-use instead (elastic#53822)
  Bump year in NOTICE.txt
  Add kibanamachine support to Github PR comments (elastic#53852)
  Add tests to ensure AAD isn't broken after performing a change on an alert / action (elastic#53333)
  Skip failing test suite
  [Vega] Sample [Flights] Airport Connections (Hover Over Airport) visualization not working (elastic#53799)
  Do not remount applications between page navigations (elastic#53851)
  [Canvas] Refactor Canvas to no longer use componentWillReceiveProps (elastic#52129)
  [Canvas] Migrate usage collector to NP plugin (elastic#53303)
jkelastic pushed a commit to jkelastic/kibana that referenced this pull request Jan 8, 2020
@poffdeluxe @elasticmachine @jkelastic
[Canvas] Refactor Canvas to no longer use componentWillReceiveProps (e…
b1b89fc 
…lastic#52129)

* Removing componentWillReceiveProps from time filter

* Changing expression form to componentDidUpdate

* Updating expression to be key-driven updates and arg_types to use compomentDidUpdate

* temporary

* Revert "temporary"

This reverts commit 255525d.

* typo fix

Co-authored-by: Elastic Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crob611 crob611 crob611 approved these changes

Assignees
No one assigned
Labels
impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:small Small Level of Effort release_note:skip Skip the PR/issue when compiling release notes review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v7.6.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@poffdeluxe @elasticmachine @kibanamachine @crob611