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.

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

fix useWindowDimensions hook #26008

Closed

Conversation

dulmandakh
Copy link
Contributor

Summary

I found useWindowDimensions hook, and thought that it would solve many issues for me, but implementation looked interesting. So I created a new project, copy pasted the code and console.log dimentions returned from the hook, then rotated iOS simulator and it's printing dimentions non-stop.

Below is my fix for useWindowDimensions, and it works for me.

Changelog

[GENERAL] [CHANGED] - fix useWindowDimensions hook

Test Plan

Copy paste the code to RN project and console.log the return value, and rotate screen it'll log new dimensions.

@dulmandakh dulmandakh requested a review from sahrens August 10, 2019 04:30
@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. labels Aug 10, 2019
Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

Code analysis results:

  • eslint found some issues. Run yarn lint --fix to automatically fix problems.

const dims = Dimensions.get('window'); // always read the latest value
const forceUpdate = React.useState(false)[1].bind(null, v => !v);
const initialDims = React.useState(dims)[0];
const [dims, setDims] = React.useState(Dimensions.get('window')); // set initial value

Choose a reason for hiding this comment

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

prettier/prettier: Delete ·

// `addEventListener` in this handler...
forceUpdate();
function handleChange({window}) {
setDims(window)

Choose a reason for hiding this comment

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

prettier/prettier: Insert ;

// `addEventListener` in this handler...
forceUpdate();
function handleChange({window}) {
setDims(window)

Choose a reason for hiding this comment

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

semi: Missing semicolon.

@dulmandakh
Copy link
Contributor Author

dulmandakh commented Aug 10, 2019

@brunolemos please review

@brunolemos
Copy link
Contributor

Yes it looks more similar to what I would write myself. But the hook was written by @sahrens so I’ll let this to him.

Copy link
Contributor

@brunolemos brunolemos left a comment

Choose a reason for hiding this comment

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

Have you confirmed Dimensions.get('window') !== Dimensions.get('window') after the change event? Personally I would prefer comparing the values, but if this works reliably then ok.

Copy link
Contributor

@brunolemos brunolemos left a comment

Choose a reason for hiding this comment

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

Tested on a react-native project and a react-native-web codesandbox, worked well.
https://codesandbox.io/embed/issue-react-native-for-web-t6qyd

@dulmandakh
Copy link
Contributor Author

dulmandakh commented Aug 10, 2019 via email

@sahrens
Copy link
Contributor

sahrens commented Aug 12, 2019

Ah yes, the issue is that when we added the bind to forceUpdate, now the dependencies for the useEffect change on every render, which means it also does the comparison with initialDims, which means we always re-render if you change the dims once.

Your implementation here is the simple obvious one, but risks missing an event and never rendering with the correct dims. I'm pretty sure if we just pass an updater function when calling forceUpdate rather than binding it on every render, everything should work.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@sahrens has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@sahrens
Copy link
Contributor

sahrens commented Aug 12, 2019

I'm going to import this but with some changes to handle lost updates.

@dulmandakh
Copy link
Contributor Author

dulmandakh commented Aug 13, 2019 via email

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @dulmandakh in 3b3c95b.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Aug 23, 2019
@dulmandakh dulmandakh deleted the fix-usewindowdimensions branch October 15, 2019 05:13
@ScreamZ
Copy link

ScreamZ commented Dec 31, 2019

@grabbou Can you confirm that mate? :) Thanks

@jfrolich
Copy link

I also ran into this. Would be great if it would be included in the next react-native release.

@Naturalclar
Copy link
Contributor

@jfrolich This change is indeed included in the 0.62 stable branch, so it should be included :)
https://github.com/facebook/react-native/blob/0.62-stable/Libraries/Utilities/useWindowDimensions.js

vishalenrique added a commit to vishalenrique/react-native that referenced this pull request May 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API: Dimensions CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. Merged This PR has been merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants