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: Avoid contentInsets overwrites scrollIndicatorInsets on iOS #13025 #17399

Closed
wants to merge 2 commits into from

Conversation

timotew
Copy link

@timotew timotew commented Dec 31, 2017

Fix to avoid contentInsets always overwrites scrollIndicatorInsets of ScrollView/ListView.
According to issue #13025

+ (void)autoAdjustInsetsForView:(UIView<RCTAutoInsetsProtocol> *)parentView

always overide scrollIndicatorInsets when called and only works when contentInsets is not set.
We only need to worry about when contentInsets is set, We only have to check and reset it after autoAdjustInsetsForView is called.

Motivation

Fix: #13025

Test Plan

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/react-native-website, and link to your PR here.)

Release Notes

…book#13025

Fix to avoid contentInsets always overwrites scrollIndicatorInsets of ScrollView/ListView.
According to issue facebook#13025 https://github.com/facebook/react-native/blob/a8391bde7d757d01521a6d12170fb9090c17a6a0/React/Views/RCTView.m#L299
always overide `scrollIndicatorInsets` when called and only works when `contentInsets` is not set.
We only need to worry about when `contentInsets` is set, We only have to check and reset it after `autoAdjustInsetsForView` is called.
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 31, 2017
@pull-bot
Copy link

pull-bot commented Dec 31, 2017

@facebook-github-bot label Android

Generated by 🚫 dangerJS

@timotew
Copy link
Author

timotew commented Jan 30, 2018

@hramos Hi there! Are these changes allowed?

@hramos
Copy link
Contributor

hramos commented Jan 30, 2018

@timotew do you have a test plan?

@timotew
Copy link
Author

timotew commented Jan 30, 2018

@hramos my team has been using this branch every since.
I will try to add a screenshot.

@brentvatne
Copy link
Collaborator

brentvatne commented Jan 30, 2018

this is definitely a bug (I repro'd it on https://snack.expo.io/S1eOAIRrM -- scrollIndicatorInsets is completely ignored if contentInset is specified). i don't think this is the best fix, though. we should probably change RCTView.autoAdjustInsetsForView so that it doesn't reset scrollIndicatorInsets to baseInset if the scrollview has a scrollIndicatorInsets prop set on it

@timotew
Copy link
Author

timotew commented Jan 30, 2018

@brentvatne that was my initial approach but I was considering the effects on other components that make use of autoAdjustInsectsForViews.

@brentvatne
Copy link
Collaborator

@timotew - I see.

I'm also concerned because scrollIndicatorInsets doesn't even seem to be in scope here? did you run this code? looks like it was made from Github directly

@timotew
Copy link
Author

timotew commented Jan 30, 2018

@brentvatne yes. I did this before the pull as you can see I had to rebase the branch then try the other approach. I later revert back to this. What fix do you think will be best and won't affect other components?

@facebook-github-bot
Copy link
Contributor

@timotew I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

@timotew timotew closed this Feb 15, 2018
@timotew timotew deleted the patch-1 branch February 15, 2018 17:26
@timotew timotew restored the patch-1 branch July 13, 2019 01:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants