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

replaced UIManager.RCTVideo > UIManager.getViewManagerConfig('RCTVideo') #1487

Merged
merged 4 commits into from
Feb 18, 2019
Merged

replaced UIManager.RCTVideo > UIManager.getViewManagerConfig('RCTVideo') #1487

merged 4 commits into from
Feb 18, 2019

Conversation

nitishxyz
Copy link
Contributor

@nitishxyz nitishxyz commented Feb 17, 2019

Describe the changes

just updated UIManager.RCTVideo > UIManager.getViewManagerConfig('RCTVideo') to remove warnings from the debug app.

Closes #1466

@nitishxyz nitishxyz changed the title replaced UIManager.RCTVideo > UIManager.getViewManagerConfig('RCTVid… replaced UIManager.RCTVideo > UIManager.getViewManagerConfig('RCTVideo') Feb 17, 2019
Video.js Outdated
@@ -225,13 +225,13 @@ export default class Video extends Component {

let nativeResizeMode;
if (resizeMode === VideoResizeMode.stretch) {
nativeResizeMode = NativeModules.UIManager.RCTVideo.Constants.ScaleToFill;
nativeResizeMode = NativeModules.UIManager.getViewManagerConfig('RCTVideo').Constants.ScaleToFill;

Choose a reason for hiding this comment

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

This will break old react native should use wrapper function. Example implementation react-native-webview/react-native-webview@263fc5e

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just made changes you suggested, thanks for the reference.

@n1ru4l
Copy link
Contributor

n1ru4l commented Feb 18, 2019

Duplicate of #1466. Could you please also apply the changes I requested there? Otherwise after you address the issues mentioned by @guhungry I am looking forward to merge your changes!

@nitishxyz
Copy link
Contributor Author

Duplicate of #1466. Could you please also apply the changes I requested there? Otherwise after you address the issues mentioned by @guhungry I am looking forward to merge your changes!

made those changes in the latest commit.

Copy link
Contributor

@n1ru4l n1ru4l left a comment

Choose a reason for hiding this comment

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

Please use this helper function to ensure backwards compat.

react-native-webview/react-native-webview@263fc5e

@nitishxyz
Copy link
Contributor Author

nitishxyz commented Feb 18, 2019

Please use this helper function to ensure backwards compat.

react-native-community/react-native-webview@263fc5e

i already did. didn't I? line 213, 234

@n1ru4l
Copy link
Contributor

n1ru4l commented Feb 18, 2019

Oh for some reason I was one commit behind sorry!

@n1ru4l
Copy link
Contributor

n1ru4l commented Feb 18, 2019

Could you please also update the changelog?

@nitishxyz
Copy link
Contributor Author

Could you please also update the changelog?

done!

Copy link
Contributor

@n1ru4l n1ru4l left a comment

Choose a reason for hiding this comment

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

@nitnk9 Thank you!

@n1ru4l n1ru4l merged commit b448b30 into TheWidlarzGroup:master Feb 18, 2019
@cobarx
Copy link
Contributor

cobarx commented Feb 19, 2019

@nitnk9 @n1ru4l This is blowing up on master for me due to UIManager not being defined. I haven't stayed up on this PR so I'm hoping one of you two understands what we need to do to import that module. I'm testing on RN 0.57.

Can you put up a PR to resolve this asap please, there's a bunch of new features that would be great to ship once we have everything sorted out.

@n1ru4l
Copy link
Contributor

n1ru4l commented Feb 19, 2019

It should be NativeModules.UIManager. Unfortunately I am on a train for the next few hours with flakey internet so I will not be able to resolve this.

@n1ru4l
Copy link
Contributor

n1ru4l commented Feb 19, 2019

Also this reminds me that we have to setup CI that does basic stuff like running eslint which could have prevented this 😅

@cobarx
Copy link
Contributor

cobarx commented Feb 19, 2019

Yes we definitely need that.

AnteWall pushed a commit to sfstudios/react-native-video that referenced this pull request Mar 1, 2019
…IManager.getViewManagerConfig('RCTVideo')` (and ensuring backwards compat) (TheWidlarzGroup#1487)

* replaced UIManager.RCTVideo >  UIManager.getViewManagerConfig('RCTVideo')

* added requested changes

* updated changelog.md

* docs: adjust wording
beauner69 pushed a commit to beauner69/react-native-video that referenced this pull request Oct 10, 2019
…IManager.getViewManagerConfig('RCTVideo')` (and ensuring backwards compat) (TheWidlarzGroup#1487)

* replaced UIManager.RCTVideo >  UIManager.getViewManagerConfig('RCTVideo')

* added requested changes

* updated changelog.md

* docs: adjust wording

(rebased from commit b448b30)
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