Fix - using the new useWindowDimensions api to fix unmounting issues #688
Conversation
src/Modal.js
Outdated
| @@ -106,7 +85,8 @@ export class Modal extends Component { | |||
| backdropStyle, | |||
| ...otherProps | |||
| } = this.props; | |||
| const { deviceHeight, deviceWidth, isVisible } = this.state; | |||
| const { isVisible } = this.state; | |||
| const { height, width } = useWindowDimensions(); | |||
There was a problem hiding this comment.
I don't think this will work because hooks can't be used in class components.
|
Thanks for working on it! I added a comment |
|
@mmazzarolo must have worked from a cache issue yeah sorry must have missed this. I'll change it to a functional component now :) |
|
@mmazzarolo re-done to lint standards and I tested it locally again but let me know if there's anything else to do. |
| onHide, | ||
| ...otherProps | ||
| }) => { | ||
| const animVal = new Animated.Value(0); |
There was a problem hiding this comment.
I think this should be a ref. See the Animated documentation.
| const animVal = new Animated.Value(0); | |
| const animVal = useRef(new Animated.Value(0)).current; |
| const animVal = new Animated.Value(0); | ||
| const { state, setState } = useState({ | ||
| isVisible: isVisible, | ||
| isMounted: false, |
There was a problem hiding this comment.
Can we move this to a ref as well? We shouldn't keep this info in the state, we don't want to trigger a re-render when we update it.
| ...otherProps | ||
| }) => { | ||
| const animVal = new Animated.Value(0); | ||
| const { state, setState } = useState({ |
There was a problem hiding this comment.
I haven't ran this code yet, but I don't think this should work. useState returns a tuple, so it should be const [state, setState] = useState(blabla). Also, let's track each state variable separately (see the suggestion).
| const { state, setState } = useState({ | |
| // I'll use a different name variable to not override the one from the props, but ideally I would instead rename the prop one | |
| const [_isVisible, _setIsVisible] = useState(isVisible); |
| setState({ | ||
| ...state, | ||
| isMounted: true, | ||
| }); |
There was a problem hiding this comment.
By using a ref for isMounted we'll be able to avoid this double render.
| } | ||
|
|
||
| return () => { | ||
| setState({ |
There was a problem hiding this comment.
And we'll avoid this one too, which btw wouldn't work here since the component is being unmounted in this flow, right?
| isMounted: true, | ||
| }); | ||
| if (state.isVisible) { | ||
| setState({ isVisible: true }); |
There was a problem hiding this comment.
Hm, I'm missing something here. If the state is already visibile, why are why setting it again to true? 🤔
| toValue: 0, | ||
| }).start(() => { | ||
| if (state.isMounted) { | ||
| setState({ isVisible: false }, onHide); |
There was a problem hiding this comment.
I don't think onHide will be invoked here. setState doesn't have a callback parameter as far as I know.
|
hey sorry yeah missed lots here, sorry working on multiple things right now but I am sorting it now. :) |
|
Hi, is this PR still active? I would love to help accelerate this |
Hi sorry currently my ability to test the branch has broken. Is there any chance you could pick these changes up when you have time as I wouldn't like to push untested code. Sorry should have kept you updated just been trying to fix the testing issues. Thanks |
|
No worries @joe-brabben i ll pick this up @mmazzarolo i have a question, this could've been fixed using |
|
@khzouroussama I think any solution is fine as long as we solve the issue without using deprecated APIs 👍 |
Overview
Unmounting causes issues with the removal of the event listener so I have used the new useWindowDimensions hook to remove the need for a listener all together as the height and width are got dynamically now.
Test Plan
I ran yarn lint and prettify for code structure testing and tested the modal works on differing screen sizes using android studio.