Comparison with other forks

[ghost]

Hello, I am just curious if anyone here has done a comparison with other forks of d-a-n's repo?

The repos that I'm looking closely at are:

• Ario-Inc/react-native-modal-picker.
• fabriziomoscon/react-native-modal-picker
• Others from this list sorted by Last update.

The first 2 repos have pulled code from this fork and this fork also has the most recent work so that's a good signal to just use this one. I'll do a diff later on today and tell you what I find here.

[peacechen]

Hopefully we can consolidate the myriad PR's and move forward improving this library. @fabriziomoscon said he was interested in contributing and I added him as a collaborator. Would you like to be added as a collaborator? This project will have the best chance at thriving when we have a group of solid contributors so that no single person has to devote full time work on it.

[ghost]

Thanks @peacechen ! I will find something to contribute first and let you know.

[ghost]

Here are the differences that I found. I only listed features the others have that this fork doesn't:

Ario-Inc Has

1. propTypes.open (a bool) which gets assigned to modalVisible in componentWillReceiveProps(). Description: "optional, boolean that triggers whether or not the dialog is shown".
2. propTypes.optionContainerStyle which is used in renderOptionList() like this: <View style={[styles.optionContainer, this.props.optionContainerStyle]}>
3. A TouchableOpacity element in renderOptionList() that looks like it is used to close the Modal when the user clicks outside of the optionContainer. (<TouchableOpacity style={styles.overlayCloseButton} onPress={this.close}></TouchableOpacity>)
4. A selectedItemStyle prop and a selectedItemTextStyle prop used in renderOption() to style an option that is selected.
5. A showCancelButton prop that is used in renderOption() to optionally display the cancel button.

Fabriziomoscon Has

1. Rendering of {option.component} field in renderOption().
2. A scrollProps prop (object) that is used to spread properties to the ScrollView in renderOptionList. Defaults to {keyboardShouldPersistTaps: 'always'}.

[ghost]

So, I just wanted to leave my notes here for now. If I find that I need any of these features, I will certainly create a pull request. Thank you.

[mikaello]

Well done, @waynebloss!

Here are some of my notes from things I have either seen in forks or thought about:

• ~propTypes.animationType. Description: optional, none/fade/slide, animation of modal~ implemented in 6eed2fa95b2fa53208cdcb0e412ef31626a95277
• Find a way to give the possibility to add a icon or similar to each option, maybe something like what fabriziomoscon have done in 9c4804ff7dd0f80eb110652600b005c752ee80be (see example). But I guess this also could be messy.
• code enhancement: remove unused transparent state
• (possibly a) code enhancement: remove BaseComponent-class and use arrow functions instead of bind

~I have also seen multiple forks that have implemented propTypes.optionContainerStyle (Ario-Inc nr 2), so that seems like a popular enhancement.~ implemented in d9ad66133dadc5b46cb29b2bd77d8a780723fa34

[peacechen]

Those are all great ideas. As the project becomes more complex, consider factoring out sub-components into separate files. In my experience, this helps a lot to optimize React performance using shouldComponentUpdate and componentWillReceiveProps.

We could create issues for each of those points to better track discussion and PR's. Someone may have a better way to implement or have experience with its pitfalls.

[mvanroon]

@mikaello regarding your notes, I don't think you should include icons. I feel like this feature might be too specific.

Personally I would really like to so a separate backdrop component, so that we can animate this independently.

By the way are there any plans to merge with a different fork and make the combined fork the official successor of d-a-n's original?

[mikaello]

I see your concern about the icon, I implemented an example with icon and it become a little messy. So until someone sends a good PR or more people requesting it could be laid on ice.

Separate backdrop seems like a cool feature.

This is already an attempt to make an official successor, and from now on pick commits / ideas and merge other forks into this one. This fork is released on NPM as react-native-modal-selector.

[mvanroon]

Right. What I meant was, perhaps we can add the features of another fork and then persuade that fork to join us instead of working on their own. Might save everyone (including dev's looking for a proper npm package) some time.