Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

feat: callback hooks for Reactified components #131

Merged
merged 3 commits into from
Apr 8, 2019

Conversation

xtinec
Copy link
Contributor

@xtinec xtinec commented Apr 7, 2019

🏆 Enhancements

Add a second argument to the exported reactify method to allow wrapped component to pass in callback hooks for React lifecycle methods. This PR only exposes the willUnmount hook. Other hooks can be added in the future.

Add a second argument to the exported `reactify` method to allow wrapped component to pass in callback hooks for React lifecycle methods. This PR only exposes the willUnmount hook. Other hooks can be added in the future.
@xtinec xtinec requested a review from a team as a code owner April 7, 2019 06:22
@codecov
Copy link

codecov bot commented Apr 7, 2019

Codecov Report

Merging #131 into master will decrease coverage by 0.1%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #131      +/-   ##
==========================================
- Coverage     100%   99.89%   -0.11%     
==========================================
  Files          77       77              
  Lines         986      988       +2     
  Branches      237      238       +1     
==========================================
+ Hits          986      987       +1     
- Partials        0        1       +1
Impacted Files Coverage Δ
...ages/superset-ui-chart/src/components/reactify.tsx 95% <50%> (-5%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4824f49...782de17. Read the comment docs.

@codecov
Copy link

codecov bot commented Apr 7, 2019

Codecov Report

Merging #131 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #131   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          77     77           
  Lines         986    988    +2     
  Branches      237    238    +1     
=====================================
+ Hits          986    988    +2
Impacted Files Coverage Δ
...ages/superset-ui-chart/src/components/reactify.tsx 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4824f49...ef635e5. Read the comment docs.

@xtinec xtinec requested review from kristw and williaster April 7, 2019 07:04
@xtinec xtinec added #enhancement New feature or request reviewable Ready for review labels Apr 7, 2019
@@ -17,6 +17,11 @@ export type ReactifyProps = {
className?: string;
};

// TODO: add more React lifecycle callbacks as needed
export type LifeCycleCallbacks = {
willUnmount?: () => void;
Copy link
Contributor

@williaster williaster Apr 8, 2019

Choose a reason for hiding this comment

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

small point -- but I think that these should mirror the actual lifecycle names because

  1. devs are already familiar with them
  2. some of them start with componentXXX and some don't (e.g., shouldComponentUpdate). The one's that don't are a little more cryptic when truncated (e.g. componentUpdate?), so if/when we add more we'll then have a mix of full names and short names which is more complicated than need be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! Let me rename this to componentWillUnmount and I'll adjust the usage in nvd3 charts in superset-ui-plugins.

Copy link
Contributor

@williaster williaster left a comment

Choose a reason for hiding this comment

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

LGTM with one comment!

@xtinec xtinec merged commit 7603520 into master Apr 8, 2019
@delete-merged-branch delete-merged-branch bot deleted the xtinec--add-willUnmountCallback branch April 8, 2019 18:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
#enhancement New feature or request reviewable Ready for review size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants