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

Allow the original content insets to be set after setting up the keyboard observer #57

Merged
merged 5 commits into from
Nov 6, 2020

Conversation

farochocolom
Copy link
Contributor

@farochocolom farochocolom commented Nov 4, 2020

Came across a case where we needed to set the insets in a viewDidLayoutSubviews() and we were setting the originalContentInset on the setupKeyboardObservers() which is convention to call on viewWillAppear().
Since viewWillAppear()gets called before viewDidLayoutSubviews(), we were setting this to nil:
keyboardScrollableScrollView?.originalContentInset = keyboardScrollableScrollView?.contentInset

This change sets the originalContentInset only when we prompt the keyboard to show and only does if we haven't set it before, or keyboardScrollableScrollView?.originalContentInset == nil

Also bumped up the deployment target but that can be reverted if necessary

@CLAassistant
Copy link

CLAassistant commented Nov 4, 2020

CLA assistant check
All committers have signed the CLA.

wmcginty
wmcginty previously approved these changes Nov 4, 2020
Copy link
Contributor

@wmcginty wmcginty left a comment

Choose a reason for hiding this comment

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

I think this needs to be a major version upgrade if we're dropping iOS versions, but 👍 for me.

@wmcginty
Copy link
Contributor

wmcginty commented Nov 4, 2020

@Specialist17 It looks like there's an issue with running the tests that needs to be looked at, and a Changelog.md entry added for the change.

@@ -636,7 +636,7 @@
DYLIB_INSTALL_NAME_BASE = "@rpath";
INFOPLIST_FILE = "Sources/Supporting Files/Info.plist";
INSTALL_PATH = "$(LOCAL_LIBRARY_DIR)/Frameworks";
IPHONEOS_DEPLOYMENT_TARGET = 9.0;
IPHONEOS_DEPLOYMENT_TARGET = 11.0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary for some piece of functionality in the iOS 11+ SDK? I would rather us leave the deployment target alone unless there is a good reason to bump it up.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's the use of adjustedContentInset instead of contentInset, but I'm not sure if that's required as part of the fix - or if just moving the setting of the variable to the first keyboard observing would do it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, @wmcginty I didn't notice the use of adjustedContentInset, but that does seem to be iOS 11+ only. @Specialist17 was it intentional to use adjustedContentInset or was the moving of the code and the nil check the main fix you were trying to do here?

if self?.keyboardScrollableScrollView?.originalContentInset == nil {
    self?.keyboardScrollableScrollView?.originalContentInset = self?.keyboardScrollableScrollView?.contentInset
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tylermilner the moving of the code and the nil check was the main idea here. I'll revert the deployment target change

Copy link
Contributor

@wmcginty wmcginty left a comment

Choose a reason for hiding this comment

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

@Specialist17 Looks like we just need a Changelog entry, and we're good to go!

tylermilner
tylermilner previously approved these changes Nov 6, 2020
@farochocolom farochocolom changed the title Moving to iOS 11 for deployment target Allow the original content insets to be set after setting up the keyboard observer Nov 6, 2020
@wmcginty wmcginty merged commit 3b89925 into BottleRocketStudios:master Nov 6, 2020
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.

4 participants