react-native-modal-selector icon indicating copy to clipboard operation
react-native-modal-selector copied to clipboard

Published 20 hours ago • verified publisher icon peacechen

Addition of multi-select feature.

Open allroundexperts opened this issue 5 years ago • 6 comments

  • Added multi select option.
  • Added husky, making auto linting possible.
  • Upgraded RN to the latest version.

PS: Messed up the previous PR, so consider this please.

allroundexperts avatar Nov 28 '19 12:11 allroundexperts

Thanks @allroundexperts This looks good, with issues from the previous PR addressed.

I would recommend against including react-native-vector-icons in this library because that requires native linking. Not everyone who uses this library may want to do that, or use that particular icon library. It would be better to design this so that the user can pass in a component (which could include an icon) instead of building that in.

peacechen avatar Dec 08 '19 02:12 peacechen

Thanks @allroundexperts This looks good, with issues from the previous PR addressed.

I would recommend against including react-native-vector-icons in this library because that requires native linking. Not everyone who uses this library may want to do that, or use that particular icon library. It would be better to design this so that the user can pass in a component (which could include an icon) instead of building that in.

@peacechen Fixed in the latest commit!

allroundexperts avatar Dec 08 '19 15:12 allroundexperts

👍 Did you update the sample app to reflect the latest changes? I'm not at my computer to test it

peacechen avatar Dec 08 '19 20:12 peacechen

👍 Did you update the sample app to reflect the latest changes? I'm not at my computer to test it

Yup!

allroundexperts avatar Dec 08 '19 20:12 allroundexperts

I'll have time to test this soon. A few more items need to be addressed:

Some of the props/methods have changed names (getSelectedItem and selectedItem). This should be documented in the Readme under Breaking Changes. Due to the above, the major version should get a bump to 2.0.0.

Tab spacing in some chunks of code have changed from 2 to 4 spaces. The eslintrc should specify tab size of 4 to prevent commits that only change tab spacing (and resultant swaths of diffs that have no functional changes).

peacechen avatar Dec 10 '19 15:12 peacechen

@allroundexperts Thank you for continuing to refine this PR. Apologies for the delay in processing this.

The sample app in master has been updated to a simpler Expo project. Please revert your changes in SampleApp, keeping only the changes in App.js and any dependencies added. You're using react-native-elements in the SampleApp which may need tweaks to the Expo project (or maybe not, most of that is automated now). It would be cleanest to revert everything in SampleApp, pull from master and rebase & squash your commits to the top of Head.

It's a bit difficult to see the big picture in this PR when there are ~20 commits. Would you mind squashing them into a single commit?

peacechen avatar May 22 '20 18:05 peacechen