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

Package Review #1358

Closed
Johan-dutoit opened this issue Apr 30, 2020 · 20 comments
Closed

Package Review #1358

Johan-dutoit opened this issue Apr 30, 2020 · 20 comments
Labels
Enhancement Help Wanted :octocat: ❓ Question Issues that are actually questions and not bug reports.

Comments

@Johan-dutoit
Copy link
Collaborator

There's been a fair amount of issues regarding the permissions with this package and it's been painful to keep up with (especially with a mostly unfamiliar codebase to me).

My plan is as follows and I'd like to get opinions before proceeding

  • Rip out all permissions and shift the responsibility to the user (we'll add best practices
  • Modernise and clean up the code base (enforcing some standards too)
  • Getting additional maintainers onboard

It's a big ask now but the package has been stale for too long. the alternative is to completely drop this package in favour of expo-image-picker.

Suggestions welcome

@Johan-dutoit Johan-dutoit added Enhancement Help Wanted :octocat: ❓ Question Issues that are actually questions and not bug reports. labels Apr 30, 2020
@Johan-dutoit Johan-dutoit pinned this issue Apr 30, 2020
@angimenez
Copy link

I like your plan 👍

@HansBouwmeester
Copy link

Leaving it to the user to sort out permissions is the way to go! There's lots of packages providing assistance, e.g. react-native-permissions.

Would be a real shame seeing this package be dropped just because the permissions jungle is getting in the way!

@tsiory
Copy link

tsiory commented Jun 15, 2020

I agreed with you.

@macavity23
Copy link

Also agree 100%. Permissions is a thing almost every serious app is going to deal with so it might as well be handled properly by react-native-permissions - this is a community project now so I think it's safe to depend on it & link to it.

@Johan-dutoit
Copy link
Collaborator Author

Johan-dutoit commented Jun 22, 2020

I started a fresh package a while ago that will eventually replace this (haven't committed it yet) that has the following opinions (created using react-native-community/bob:

  • Swift for the iOS Package
    • This should use the new PHPicker
  • Kotlin for the Android Package
  • TypeScript for the JavaScript package

That being said, I don't have the time to fully commit to this now. If you are keen to assist, please reach out.

@ravirajn22
Copy link
Contributor

@Johan-dutoit appreciate your effort. But trying to completely rewrite the library will take huge effort and has high percentage of failure to complete.

I have the following recommendation,

  • Let permission be within the library for iOS, for android we have permission android built in to react-native. We should not force lib users to add another third party library for the sake of permission.
  • Lets rewrite file by file from objective C to swift. Swift is far-far better language to grasp than objective c.
  • Whereas in case of Android lets stick with Java because there is not much advantage you gain on moving to Kotlin from what I know just few syntactic sugars.

I and many others are ready to contribute if there is guarantee that the pull request will be reviewed, merged to master or rejected in quick time. It should not be left stale.

I am keen to assist, how do we move forward?

@Johan-dutoit
Copy link
Collaborator Author

@ravirajn22 some good points brought up.

I created a new branch for you (and whoever wants to help), called vnext.

PR's into this branch will be reviewed and merge if all looks good.

@douglasjunior
Copy link

About expo-image-picker, I don't think that is a good idea for bare React Native projects, because it require react-native-unimodules.

@ravirajn22
Copy link
Contributor

@Johan-dutoit I suggest we remove ImagePicker.showImagePicker, people if they want can show menu of their choice like using ActionSheetIOS from react-native or simple buttons etc. It's a burden for this library.
Community your thoughts?

@Johan-dutoit
Copy link
Collaborator Author

@ravirajn22 Totally agree.

Especially with trying to support all the theming of the action sheet.

Having the following two functions would suffice:

  • launchCamera, and
  • launchImageLibrary

As a nice to have, I'll add an example to the example project that uses react-native-action-sheet.

Let's do this on the vnext branch.

@tastafur
Copy link

tastafur commented Aug 25, 2020

Other suggestions to improve @Johan-dutoit , the use of the camera when it is in video mode, the only data we give is the video file but it would also be necessary to know the format, the video quality, resolution and more parameters

@Johan-dutoit
Copy link
Collaborator Author

Johan-dutoit commented Oct 8, 2020

For those interested @ravirajn22 has done a ton of work related to this (which is highly appreciated). These changes live on the vnext branch.

Testing and feedback is greatly welcomed!

Once sufficient feedback has been received (and testing of course), then we can promote to master and release an alpha on npm (to reach a wider audience).

Any issues against vnext should be tagged with the vnext label.

@ravirajn22
Copy link
Contributor

ravirajn22 commented Oct 8, 2020

Also list here any master branch feature unavailable on vnext branch which is a deal breaker to upgrade. We will brainstorm and consider to add to vnext.

@ravirajn22
Copy link
Contributor

@Johan-dutoit there is no comments from users (I think no one has tried vnext branch). What shall we plan next? release a new major beta version using the vnext branch. We should maintain both branches parallel and release versions for sometime and update master docs to both releases.

@ravirajn22
Copy link
Contributor

ravirajn22 commented Oct 29, 2020

@Johan-dutoit can you provide manage issue, write permission (for vnext branch) for me. So that I can push my changes and handle issues promptly. Any major API changes will obviously go through review process.

Also we can close this issue.

@AronBe
Copy link

AronBe commented Nov 2, 2020

Hi, what about adding a customButton to the vnext as that seems currently the way to allow users to have an option to upload an image alongside e.g. a pdf document through react-native-document-picker

@ravirajn22
Copy link
Contributor

@AronBe I don't get your feature request. What customButton?

@AronBe

This comment has been minimized.

@Johan-dutoit
Copy link
Collaborator Author

@AronBe please create a new issue with as much detail as possible. It's off topic for this issue and won't easily be able to manage/track it in here.

@Johan-dutoit
Copy link
Collaborator Author

Closing this, as tons of progress has been made regarding this issue.
Many thanks to everyone involved, especially @ravirajn22.

@react-native-image-picker react-native-image-picker locked as resolved and limited conversation to collaborators Nov 3, 2020
@Johan-dutoit Johan-dutoit unpinned this issue Nov 3, 2020
@Johan-dutoit Johan-dutoit pinned this issue Nov 3, 2020
@Johan-dutoit Johan-dutoit unpinned this issue Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Enhancement Help Wanted :octocat: ❓ Question Issues that are actually questions and not bug reports.
Projects
None yet
Development

No branches or pull requests

9 participants