peacechen / react-native-modal-selector

A cross-platform (iOS / Android), selector/picker component for React Native that is highly customizable and supports sections.
MIT License
369 stars 129 forks source link

onChange not called on ios #97

Closed danielzzz closed 4 years ago

danielzzz commented 5 years ago

hi,

the onchange callback is never called on my system (ios). it works perfectly on android.

using version 1.0.0

thanks, dan

return (<ModalSelector style={{ height: 1, width: 1, position: 'absolute', borderWidth: 1, borderColor: 'red' }}
                             visible={this.state.showModal}
                             data={this.state.photoOrigins}
                             onChange={() => {
                                         alert('asdasd'); // THIS ONE IS NOT CALLED
                                       }}
                             onModalClose={() => {
                                             this.setState({
                                               showModal: false
                                             })
                                           }} />)
peacechen commented 5 years ago

This seems puzzling as my app is successfully using onChange for both iOS and Android. I wonder if a recent version of RN changed or broke this. What version of RN is your app using?

danielzzz commented 5 years ago

yes, just the last one

 "react": "^16.6.3",
    "react-dom": "^16.4.1",
    "react-native": "^0.57.8",

I was checking the source but could not figure out the problem. also I was using 0.0.27 before that. I thought it might have been something with the library, but upgrading to 1.0.0 didn't help

is there any workaround I could try? like picking the selected item on close?

danielzzz commented 5 years ago

I ended up using a basic react native modal component. xxx, dan

cultofmetatron commented 5 years ago

running into this myself with expo

peacechen commented 5 years ago

@cultofmetatron Is your project also using RN 0.57 ?

romualdleuga commented 5 years ago

I have the same issue here, any solution?

peacechen commented 5 years ago

There may be a race condition in the onDismiss callback. https://github.com/peacechen/react-native-modal-selector/blob/master/index.js#L259

Will need to set a breakpoint there to see what's going on.

peacechen commented 5 years ago

I updated the sample app to RN 0.57.8 and it's firing the onChange callback. Note that the sample app is ejected. Are the ones having this issue all using Expo?

Follow the instructions in the SampleApp folder and start the demo app. Set breakpoints here: https://github.com/peacechen/react-native-modal-selector/blob/master/SampleApp/App.js#L50

and here: https://github.com/peacechen/react-native-modal-selector/blob/master/SampleApp/App.js#L59

The onChange callbacks are fired for both modal selectors.

Edit: for the failing cases, is the closeOnChange prop set to false?

Graren commented 5 years ago

Hi, I'd like to report the same as the other guys, onChange is not firing on IOS running on React 16.3.1 and react-native 0.55.4 (No expo).

I had not specified closeOnChange so it defaults to true, I guess onDismiss is not getting called, or the changedItem is undefined by the time it get's called

peacechen commented 5 years ago

@Graren Would you try running the SampleApp?

Graren commented 5 years ago

@peacechen I have to apologize, the error was on the function passed as the onChange prop, the lib works beautifully both on android and IOS.

hitchcocknz commented 5 years ago

I am running RN 0.58.4 (no Expo) and I don't see the onChange firing under IOS but same code does under Android.

danielzzz commented 5 years ago

hello, just ran into this issue again with some old code... but it's still not working on ios with rn 0.61-rc3 and this lib version 1.1.1

strangely, onChange callback is called a while after the modal is closed, but it my case is not helping because the other dependent code has been executed already with old values.

cheers, dan

peacechen commented 5 years ago

I believe that newer versions of RN have changed the modal-close callback on iOS. Would someone be up for submitting a PR?

evanlesmez commented 4 years ago

Also ran into this today

"react-native-modal-selector": "^1.1.4",
"react": "16.8.3",
"react-native": "https://github.com/expo/react-native/archive/sdk-35.0.0.tar.gz",
"expo": "^35.0.0",
peacechen commented 4 years ago

@evanlesmez Please post your <ModalSelector> usage.

jacobkring commented 4 years ago

I'm using react-native 0.61.5 and I ran into this issue today. I previously was using this library without an issue but I haven't tested some of these things in a while and just noticed this.

I forked the repo and the issue seems to be related to this pull request.

When onDismiss is called on iOS the changedItem state is undefined? I didn't dig much further since I was able to solve the problem in my fork but I assume this would break for other versions of RN.

I changed the code in onChange and it's working for my use case without any issues, like this:

onChange = (item) => {
        this.props.onChange(item);
        /* this doesn't seem to play nice, the setState below doesn't propagate to the onDismiss state (changeItem) is undefined in the onDismiss function
        if (Platform.OS === 'android' || (Modal.propTypes !== undefined && !Modal.propTypes.onDismiss)) { // don't know if this will work for previous version, please check!
            // RN >= 0.50 on iOS comes with the onDismiss prop for Modal which solves RN issue #10471
            this.props.onChange(item);
        }*/
        this.setState({ selected: this.props.labelExtractor(item), changedItem: item }, () => {
          if (this.props.closeOnChange)
            this.close(item);
        });
    }

For more context my ModalSelector looks like this:

      <ModalSelector
            cancelText={cancelText}
            style={{ flex: 1 }}
            visible={visible}
            onModalClose={onModalClose}
            backdropPressToClose
            componentExtractor={(data) => {
              if (data.icon) {
                return (
                  <View key={data.id} style={{ flexDirection: 'row', alignItems: 'center' }}>
                    <Image source={data.icon} style={styles.imageStyle} />
                    <Text style={styles.textStyle}>{data.label}</Text>
                  </View>
                )
              }
              return <Text key={data.id} style={styles.textStyle}>{data.label}</Text>
            }}
            disabled={disabled}
            data={options}
            onChange={(option) => {
              // this function was never called on iOS
              //do stuff
            }}
          >
peacechen commented 4 years ago

Thanks @jacobkring for finding a solution. It looks like props.onChange() should be called for both iOS and Android. It should be safe to do that, but I'm worried that it may cause problems with prior versions of RN on either platform.

Does onModalClose receive the selected item? Is onChange required for your use case in addition to that?

jacobkring commented 4 years ago

I haven't looked at onModalClose, tbh. I didn't think about using that. I can try that out tomorrow. I'm controlling the visibility of the modal elsewhere so I'm just using the onModalClose to clear that state when someone clicks cancel or on the backdrop.

peacechen commented 4 years ago

From inspecting the code, onModalClose should return the selected item.

I'm ok with your solution of calling props.onChange() without checking for Android or existence of onDismiss. The only risk I can see is it could be fired an extra time on older RN versions. Would you mind submitting a PR?

jacobkring commented 4 years ago

Sure I can submit a PR and I will test the onModalClose today. Does it need to be called in onDismiss? Wouldn't it avoid duplication if you don't call it there?

That was the part I wasn't sure about.

peacechen commented 4 years ago

Let's leave it in onDismiss for now. Yes there's a chance it could get called twice, but that's the lesser of two evils. It's a shame FB didn't fully abstract the platform differences for Modal.

peacechen commented 4 years ago

Thanks for the PR @jacobkring

Published in v2.0.0