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

Support iOS 11 prefersLargeTitles #1643 #2090

Merged
merged 3 commits into from
Nov 20, 2017
Merged

Conversation

yomybaby
Copy link
Contributor

Please, see #1643

Copy link
Contributor

@eliperkins eliperkins left a comment

Choose a reason for hiding this comment

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

Nice!

NSNumber *prefersLargeTitles = self.navigatorStyle[@"prefersLargeTitles"];
BOOL prefersLargeTitlesBool = prefersLargeTitles ? [prefersLargeTitles boolValue] : NO;
if (prefersLargeTitlesBool) {
[self.navigationController.navigationBar setPrefersLargeTitles:YES];
Copy link
Contributor

@eliperkins eliperkins Nov 3, 2017

Choose a reason for hiding this comment

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

These setters can be written as self.navigationController.navigationBar.prefersLargeTitles = prefersLargeTitlesBool as well, to use Objective-C 2.0 syntax, as well as reuse the prefersLargeTitlesBool variable to match/simplify the logic.

@@ -644,6 +644,16 @@ -(void)setStyleOnAppearForViewController:(UIViewController*)viewController appea
self.navigationItem.titleView.clipsToBounds = YES;
}
}

if ([[[UIDevice currentDevice] systemVersion] floatValue] >= 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.

What do you think about using a if (@available(iOS 11.0, *)) check here instead of checking the device's system version?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since the @available can only be used in xcode9 and higher (backward compatible wise), it's better to use device's system version

Copy link
Contributor

Choose a reason for hiding this comment

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

If the argument is for backwards compatibility, I would argue that using [self.navigationController.navigationBar respondsToSelector:@selector(setPrefersLargeTitles:)] would be a better alternative than a version check.

@eliperkins
Copy link
Contributor

What can I do to help get this PR merged in?

@gran33
Copy link
Contributor

gran33 commented Nov 12, 2017

@eliperkins can u plz fix it to
[self.navigationController.navigationBar respondsToSelector:@selector(setPrefersLargeTitles:)]
?

@eliperkins
Copy link
Contributor

@gran33 Since I'm just a contributor and not a collaborator, I can't make edits to this PR.

@yomybaby can you make that change?

Otherwise, I'll need to open a different PR or we can merge this and fix it in master.

@yomybaby
Copy link
Contributor Author

yomybaby commented Nov 14, 2017 via email

@yomybaby
Copy link
Contributor Author

yomybaby commented Nov 14, 2017

@eliperkins I just changed some code. Thank for your suggestion.

@birkir
Copy link
Contributor

birkir commented Nov 17, 2017

The ScrollView behaviour is a little weird.
The complete animation happens on like 5 pixel scroll.

Works fine if the scrollview content has less height than the scrollview itself.

Works vs Does not

@eliperkins
Copy link
Contributor

@birkir I don't think this is an issue with this library, but rather just an iOS quirk.

If you find the root cause of this is in react-native-navigation, I would encourage you to open a separate issue.

@birkir
Copy link
Contributor

birkir commented Nov 17, 2017

Yeah I'm more suspecting React Native in general.

Maybe this recently added method can be used to fix the problem

@icodesign
Copy link

@yoavaa @eliperkins Should we get it merged?

@gran33 gran33 merged commit 9f2f15b into wix:master Nov 20, 2017
gran33 added a commit that referenced this pull request Nov 21, 2017
gran33 added a commit that referenced this pull request Nov 21, 2017
@birkir
Copy link
Contributor

birkir commented Nov 21, 2017

What happened? @gran33

@gran33
Copy link
Contributor

gran33 commented Nov 21, 2017

Sorry guys, This PR breaks xcode8 users, and force xcode8 to update to xcode9.

@eliperkins
Copy link
Contributor

Can we open a discussion to updating this library to require Xcode 9? It seems like quite a big deal to cut features of the most recent operating system in order to support an outdated toolchain.

@brunolemos
Copy link

brunolemos commented Nov 21, 2017

Xcode 9 is the latest stable version, that's what we should support.
Also, people can have multiple versions of Xcode on their machine.

@gran33
Copy link
Contributor

gran33 commented Nov 22, 2017

@yomybaby can u plz try to build it in xcode8 and add

#ifdef __IPHONE_11_0

YOUR_CODE_HERE

#endif

@birkir
Copy link
Contributor

birkir commented Nov 22, 2017

About the scroll issue... I think you may be right about it being a iOS quirk. I saw one app that has the same bug Parcel (I don't know if it's an react native app or not).

But somehow Apple makes it look smooth in their apps, I deeply want to fix the problem, so any ideas are greatly appreciated, I don't know where to look or start debugging.

@dkast
Copy link

dkast commented Nov 25, 2017

Any updates on this issue?

@guyca
Copy link
Collaborator

guyca commented Nov 26, 2017

@yomybaby Sorry to trouble you, could you resubmit the PR with support for xcode 8?

@brunolemos
Copy link

@guyca I think if you git cherry-pick his commit add add the xcode 8 support would be enough, if he is not available

thanhzusu pushed a commit to thanhzusu/react-native-navigation that referenced this pull request Nov 27, 2017
* master: (22 commits)
  setTabButton to change the label of the TabButton (wix#2215)
  3D Touch Preview API (wix#2186)
  Update android-specific-use-cases.md
  Update android-specific-use-cases.md
  Load props from props registry for redux screens
  Nav bar save props (wix#2211)
  Support passing unserializable props to custom navBar component
  Save buttons props for root screens
  Ensure that styles set on individual buttons are not overridden (wix#2200)
  Update top-level-api.md
  (Android) Allow disableOpenGesture right or left in the drawer (wix#2189)
  Revert "Support iOS 11 prefersLargeTitles #1643 (wix#2090)" (wix#2204)
  Support iOS 11 prefersLargeTitles #1643 (wix#2090)
  Support passing unserializable props to custom button (wix#2192)
  addOnNavigatorEvent improvements (wix#2175)
  Fix missing props (wix#2179)
  Revert "Don't custom button props over the bridge (wix#2174)" (wix#2177)
  Don't custom button props over the bridge (wix#2174)
  Set missing TopBar background color
  Register multiple navigatorEventListeners (wix#2173)
  ...
@anonrig
Copy link

anonrig commented Nov 28, 2017

It seems that this PR is reverted in master, any ETA on the status? I couldn't keep track.

@brunolemos
Copy link

brunolemos commented Dec 13, 2017

Does anyone know where can I add this code on v2?

--
I'm using patch-package and picking some much needed PRs like #1836 and #1816.

garrettm pushed a commit to Ginger-Labs/react-native-navigation that referenced this pull request Dec 19, 2017
* Support iOS 11 prefersLargeTitles #1643

* better and more simple version checking and setting
garrettm pushed a commit to Ginger-Labs/react-native-navigation that referenced this pull request Dec 19, 2017
garrettm pushed a commit to Ginger-Labs/react-native-navigation that referenced this pull request Dec 19, 2017
* Support iOS 11 prefersLargeTitles #1643

* better and more simple version checking and setting
garrettm pushed a commit to Ginger-Labs/react-native-navigation that referenced this pull request Dec 19, 2017
@doertli
Copy link

doertli commented Jan 25, 2018

Is this PR likely to get remerged anytime soon?

@gran33
Copy link
Contributor

gran33 commented Jan 26, 2018

Yes, will update.
cc: @yogevbd @guyca

@brunolemos
Copy link

It was already remerged yesterday on #2593 AFAIK.

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.

10 participants