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

Refactor the image zoom component to avoid the use of deprecated methods #127

Closed
wants to merge 1 commit into from

Conversation

TPXP
Copy link

@TPXP TPXP commented May 7, 2020

componentWillMount and componentWillReceiveProps are deprecated lifecycle methods and should be avoided. This PR offers to move the componentWillMount part to the constructor (which has the exact same effect), and moves the componentWillReceiveProps part to componentDidUpdate, which is safe since the function performs comparison between props to detect any change (thus the additional calls will not result in unexpected behaviour).

Diff kinda messed up because I removed one indentation as I moved the pan responder to the constructor.

Fixes #111 and closes #114.
After merging this PR, consider releasing a new version of this module as well as https://github.com/ascoders/react-native-image-viewer to fix ascoders/react-native-image-viewer#360

…onentWillMount and componentWillReceiveProps
@pxpeterxu
Copy link

pxpeterxu commented May 12, 2020

@TPXP: thanks a ton for putting the work in here! I ran into the same issues.

Might it be possible for this to use static getDerivedStateFromProps rather than componentDidUpdate as the replacement for componentWillReceiveProps? (Performance-wise, this would require only one re-render rather than two)

Edit: I see that the componentDidUpdate() calls centerOn, which is an animation, so it's the right place for it!

Also, it might be a bit easier to review this if you moved the panResponder initialization into a constructor() (by renaming componentWillMount to just

constructor(props: Props) {
  super(props);
  this.panResponder = ...;
}

@ArtemKolichenkov ArtemKolichenkov mentioned this pull request May 15, 2020
1 task
@ArtemKolichenkov
Copy link
Collaborator

@TPXP nice work!
I've started some cleaning in this repo though and unfortunately now your branch has conflicts. I refactored the methods in PR #130 and trying to release it ASAP so I will close your PR, sorry.
I will make sure to mark your contributions once I have allcontributors bot installed in this repo (can't do it myself, need @ascoders to do it or make it old-fashioned manual way).

P.S. If you're interested what's going on with this project - read #129

@ArtemKolichenkov
Copy link
Collaborator

@all-contributors please add @kingdaro for code

@allcontributors
Copy link
Contributor

@ArtemKolichenkov

I've updated the pull request to add @kingdaro! 🎉

@ArtemKolichenkov
Copy link
Collaborator

oops, copy pasted from other PR 😅
@all-contributors please add @TPXP for code

@allcontributors
Copy link
Contributor

@ArtemKolichenkov

I've put up a pull request to add @TPXP! 🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants