search

lightbasenl / react-native-panorama-view

A simple component for displaying panoramic images in your React Native app.
MIT License
127 stars 47 forks source link

IOS Swift migration #49

Closed cristianoccazinsp closed 3 years ago

cristianoccazinsp commented 3 years ago

With the codebase being in swift, there's no need to some of the conditional header imports. However, a full clean + build may be required after a pod install to remove old artifacts.

cristianoccazinsp commented 3 years ago

I think it can't contan "@ ... /" characters, since that's what the pod will then use to link the project and stuff.

The requirement is basically that the pod name matches the podspec file, I believe. Feel free to test other naming alternatives, but when I looked at other RN packages, that also seemed to be the pattern.

rodymolenaar commented 3 years ago

I think it can't contan "@ ... /" characters, since that's what the pod will then use to link the project and stuff.

The requirement is basically that the pod name matches the podspec file, I believe. Feel free to test other naming alternatives, but when I looked at other RN packages, that also seemed to be the pattern.

Ah okay. Just wanted to check!

Feel free to merge once you're ready.

cristianoccazinsp commented 3 years ago

Can you tell me a bit more about the steps required to test this when installed directly with npm from a git repository? I cannot seem to get the "dist" folder to get built. I wanted to give it one last test before merging, but installing from git doesn't work due to the build steps.

rodymolenaar commented 3 years ago

Can you tell me a bit more about the steps required to test this when installed directly with npm from a git repository? I cannot seem to get the "dist" folder to get built. I wanted to give it one last test before merging, but installing from git doesn't work due to the build steps.

The build step is generating the JavaScript and types from the TypeScript source. You can choose to run yarn && yarn build inside node-modules/@lightbase/react-native-panorama-view or use yarn link to connect a local repo to your project and run yarn build there.

Not sure what the equivalents are for NPM.

cristianoccazinsp commented 3 years ago

I think I got it, I still had to run the build step and manually copy the dist folder into my node_modules own project. I guess that's acceptable at least until it is published to npm. Thanks!

cristianoccazinsp commented 3 years ago

Since one of the changes included before was to be able to download images remotely, it makes sense to now return some extra info / messages when the loading fails. You are right about the Android counterpart, totally overlooked that one!

Do you think there's no point in returning the reason behind a load failure?

rodymolenaar commented 3 years ago

Since one of the changes included before was to be able to download images remotely, it makes sense to now return some extra info / messages when the loading fails. You are right about the Android counterpart, totally overlooked that one!

Do you think there's no point in returning the reason behind a load failure?

I'm totally for providing more information on why an error occured, but maybe we should implement it in a way that is the same for both Android & iOS.

For example, a network connection loss error might be completely different between the two. So a user would have to write a bunch of code just to use the extra information.

cristianoccazinsp commented 3 years ago

That makes sense. Then the error should probably be a code string or something.

cristianoccazinsp commented 3 years ago

I've reverted the error message change on the JS/TS side in order to work on that later, but left the native side initial work for both Android (new) and iOS. Could use some help thinking in the error codes since iOS and Android paths can be different :)

Also removed the lock file.

chernandezq commented 3 years ago

You can review this, I have problems with the pod installation [!] No podspec found for@lightbasein../node_modules/@lightbase/react-native-panorama-view``

cristianoccazinsp commented 3 years ago

In this PR I technically fixed the podspec error.

El sáb., 27 de febrero de 2021 11:54, Cristian Hernández < notifications@github.com> escribió:

You can review this, I have problems with the pod installation [!] No podspec found for @lightbase https://github.com/lightbasein ../node_modules/@lightbase/react-native-panorama-view``

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/lightbasenl/react-native-panorama-view/pull/49#issuecomment-787084212, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALU263G4MN2ZP4CO2F3YM7DTBEBRZANCNFSM4YDF2E3A .

chernandezq commented 3 years ago

@cristianoccazinsp How I can reply to this solution on my local machine? I already changed the ios of your commits files in my local node_modules but I have the same error, can you help me

cristianoccazinsp commented 3 years ago

Install directly from git. Add to package.json:

"@lightbase/react-native-panorama-view": "github:lightbasenl/react-native-panorama-view#swift-migration",

You will most likely need to also build the TS files and copy them into the dist folder.

For me the podspec change worked fine. Make sure to clean the the xcode project, and also pod install. From the error above, you are still using the old podspec file.

cristianoccazinsp commented 3 years ago

@rodymolenaar do you think it is good to merge now? Probably good idea to do another canary release with this so pod install issues are resolved too. An important step is to clean iOS' build folder from XCode.

rodymolenaar commented 3 years ago

This PR has been merged and 1.0.0-canary.1 released