Skip to content

Added comment explaining what FIX_SHIFT does #105

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

Merged
merged 2 commits into from
Apr 23, 2021

Conversation

tecno40
Copy link

@tecno40 tecno40 commented Apr 21, 2021

This comment was mostly copied from a comment on another PR #101 . I believe adding it to the code will make it easier for other developers to understand what FIX_SHIFT is doing in the code.

@tecno40
Copy link
Author

tecno40 commented Apr 21, 2021

Also I didn't address this in the current PR, but while reading over the code again I noticed FIX_SHIFT is being set to the window height via export var FIX_SHIFT = Dimensions.get('window').height * 2. Could this potentially create an issue when they rotate the phone as the height would change, or does the x 2 end up taking care of that in most cases? I did go and look at a few phones screen sizes and it looks like the width is generally half the height. Sometimes it's a bit less, but it's generally only off by a few pixels in those cases.

@SteffeyDev SteffeyDev changed the base branch from master to develop April 23, 2021 02:28
@SteffeyDev SteffeyDev merged commit 41d1a3a into SteffeyDev:develop Apr 23, 2021
@SteffeyDev
Copy link
Owner

Comment is helpful, thanks! The actual value of FIX_SHIFT doesn't really matter, as long as it is off screen we are fine. I could have done 10000000 as the value, and in theory it would be the same. I think Dimensions height is always the height in portrait mode, and does not change on rotation, but I could be wrong on that. No one has reported an issue yet in practice, so I'm assuming it's working out. Hoping to get rid of FIX_SHIFT sooner than later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants