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 for RN 0.40 #146

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
@natorojr

This comment has been minimized.

natorojr commented Jan 24, 2017

Dear @yamill,

First of all, thank you for your contributions to the open source community.

I can understand that you may be preoccupied with your other projects, but I would greatly appreciate if you could merge in this PR, which includes the changes needed to support React Native 0.40+. As you can see, the changes are very straightforward (2 lines).

I'm using this library in a few key projects, but am unable to upgrade to React Native 0.40 due to these breaking changes. I'd prefer not to fork the project, as I am personally against fragmentation / duplication in the community and would rather continue advocating your library and the excellent work you've already done.

Thank you in advance for your consideration.

@manask88

This comment has been minimized.

manask88 commented Jan 24, 2017

+1

4 similar comments
@thomasobrien99

This comment has been minimized.

thomasobrien99 commented Jan 24, 2017

+1

@realjamie

This comment has been minimized.

realjamie commented Jan 25, 2017

+1

@carolinempapp

This comment has been minimized.

carolinempapp commented Jan 25, 2017

+1

@danlopez

This comment has been minimized.

danlopez commented Jan 25, 2017

👍

@georgiana-gligor

This comment has been minimized.

georgiana-gligor commented Jan 26, 2017

If you need to use RN 0.40 asap, there's a quick solution that you can use in your project today, until the author merges this pull request.

Go to package.json and search for the dependency that looks similar to

"react-native-orientation": "^1.17.0",

Replace it with:

"react-native-orientation": "git+https://github.com/jedt/react-native-orientation.git",

This will effectively point your package manager to the pull request author's master branch.

@almirfilho

This comment has been minimized.

almirfilho commented Feb 14, 2017

+1

@manask88

This comment has been minimized.

manask88 commented Feb 14, 2017

this is great @landonalder . When can we expect this PR to be merged and a new npm package? If you guys are overwhelmed with other projects, I can offer my self to help here :)

@@ -4,7 +4,7 @@
#import <Foundation/Foundation.h>
#import <UIKit/UIKit.h>
#import "RCTBridgeModule.h"
#import <React/RCTBridgeModule.h>

This comment has been minimized.

This comment has been minimized.

@manask88

manask88 Mar 10, 2017

@jedt , I think max suggestion is good. That way, the library can be backwards compatible. Do you think you can update your PR soon? If you need any help, let me know!

This comment has been minimized.

@fungilation

fungilation Apr 29, 2017

Contributor

RN is going to be 0.44 in a few days. I prefer this over redundant code for legacy support 4+ versions back.

@ugiacoman

This comment has been minimized.

ugiacoman commented Apr 17, 2017

Can confirm this PR works with RN v "0.42.3". Please merge!

@fungilation

fungilation approved these changes Apr 29, 2017 edited

Should there be an official fork, as this package owner is MIA since August 2016?

I verified this patch works on RN 0.43.4, as well as 0.44.0.

@@ -4,7 +4,7 @@
#import <Foundation/Foundation.h>
#import <UIKit/UIKit.h>
#import "RCTBridgeModule.h"
#import <React/RCTBridgeModule.h>

This comment has been minimized.

@fungilation

fungilation Apr 29, 2017

Contributor

RN is going to be 0.44 in a few days. I prefer this over redundant code for legacy support 4+ versions back.

@ugiacoman

This comment has been minimized.

ugiacoman commented Apr 29, 2017

@fungilation Yes, I do think someone should make an official fork. Someone who is active and willing to merge PRs haha

@andrerfneves

This comment has been minimized.

Collaborator

andrerfneves commented May 20, 2017

5a16ac8 was merged and solves compatibility issues with new RN versions.

@fungilation

This comment has been minimized.

Contributor

fungilation commented May 20, 2017

@andrerfneves, should merge this patch instead with eliminating unnecessary HEADER_SEARCH_PATHS.

@andrerfneves

This comment has been minimized.

Collaborator

andrerfneves commented May 20, 2017

@fungilation good catch. Just pushed a hotfix that solves this issue: 01feaaa

@fungilation

This comment has been minimized.

Contributor

fungilation commented May 20, 2017

Great. Even better if it gets bumped to npm 😉 So we don't have to use our own patch work of forks or git in yarn

@andrerfneves

This comment has been minimized.

Collaborator

andrerfneves commented May 20, 2017

I don't currently have access to the NPM package to publish it myself. I'll work with the package owner to get him to publish it when the updates have all been completed. @fungilation for now, let's keep the package pointed to master on github. I'll make sure to add a note on README as well.

@fungilation

This comment has been minimized.

Contributor

fungilation commented May 20, 2017

Awesome. Thanks for bringing this package back to life!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment