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

iOS onChange called twice #140

Open justincrow opened 4 years ago

justincrow commented 4 years ago

Since version 2, as documented, iOS onChange is called multiple times. Please could you explain why this decision was made and how to work around it?

peacechen commented 4 years ago

onChange wasn't called for some users. What version of RN are you using? There was a breaking change in RN around 0.5 or 0.6 that necessitated the recent PR. We could add a prop to control this behavior so that it doesn't get called multiple times.

justincrow commented 4 years ago

Sorry, should have said, we're on "0.61.5". I appreciate this was a fix for some users - unfortunately it's proved hard for us to prevent multiple callbacks. Any help appreciated. Thanks.

peacechen commented 4 years ago

There are unfortunately too many variations in RN's modal behavior of onDismiss. It may be better to not use that internally.

Would you mind testing these options:

  1. Delete the call to onDismiss here. Easiest way would be to edit node_modules/react-native-modal-selector/index.js
  2. Use the prop onModalClose instead of onChange.
justincrow commented 4 years ago

Hi, I've tried the above, but with no success. Deleting the call to onDismiss just dismisses the dialog when clicked, with no callbacks. Using onModalClose instead of onChange has the same effect. Both changes together also give the same effect. Thanks

peacechen commented 4 years ago

That is surprising. onChange should be called every time a TouchableOpacity is pressed. In the case that it doesn't call onChange, are you selecting an item or pressing "cancel"? Please paste your <ModalSelector> usage in case there are any other props interfering.

justincrow commented 4 years ago

To break this down: If I comment out line 313 of index.js, then the onChange gets called but the resultant dialog that we open (to select a document/photo) gets closed immediately.

<ModalSelector
      style={styles.thumbnailContainerStyle}
      data={fileOptions}
      onChange={onChange}
      closeOnChange
      cancelText="Cancel"
      optionTextStyle={styles.optionsText}
      optionContainerStyle={styles.optionContainer}
      cancelTextStyle={styles.cancelText}
      cancelStyle={styles.cancelContainer}
    >

And our onChange is:

const onChange = (option) => {
    switch (option.key) {
      case 0:
        ImagePicker.openCamera(...)
        break;
      case 1:
        ImagePicker.openPicker(...)
        break;
      case 2:
        DocumentPicker.pickMultiple(...)
        break;
      default:
        break;
    }
  };

If I don't comment out that line, then our onChange gets called twice, so for instance the document picker dialog opens, closes, then re-opens.

If I don't comment out that line, and use 'onModalClose' instead of 'onChange', the behaviour is the same as commenting out the line ie our dialog opens, then immediately closes.

If I both comment out the line and use 'onModalClose', the behaviour is the same as above.

peacechen commented 4 years ago

Is it closing after you select an option, or immediately after the modal opens? If you don't want the modal to close upon a selection, use the prop closeOnChange={false}. Your code passes in that prop without a value which makes it true. https://reactjs.org/docs/jsx-in-depth.html#props-default-to-true

If that's not the case, this is very puzzling. onDismiss isn't a required prop, so omitting that shouldn't cause the modal to close on its own. https://reactnative.dev/docs/modal/#ondismiss

Does ImagePicker.openPicker() open this modal?

pr4nta commented 4 years ago

Yes, it's closing immediately after an option is selected. closeOnChange={false} fixes this immediate closing when onDismiss is commented out but in that case, I need to close the modal manually by pressing "cancel", which can be annoying for the user.

peacechen commented 4 years ago

@dx-pranta-bot This modal is designed to close when an option is selected. Since closeOnChange={false} doesn't do what you need it to, what different behavior would you like?

peacechen commented 4 years ago

@justincrow I re-read your notes and think I understand what I missed earlier. It sounds like you use <ModalSelector> to open another dialog (e.g. Document picker) and want that second dialog to remain open. However if <ModalSelector> closes, the Document picker also closes.

<ModalSelector> is designed by default to close on selection. In your use case, use the prop closeOnChange={false} to prevent it from closing on selection. You'll then need to close the modal after the picker is done. The visible prop allows for manual control of the modal open/close state. In your case it may be more convenient to imperatively close it. To do that, obtain a ref to the <ModalSelector> instance and call its close() method.

PS: What are the values passed to the onChange callback when it's fired multiple times?

justincrow commented 4 years ago

Sorry for the radio silence. The values passed to the onChange are the same on both 1st and 2nd callbacks eg {"key": 1, "label": "Photo library"}

If we use closeOnChange={false}, then the first callback successfully opens our (eg Document Picker) dialog, but duplicate callback is delayed until we close the dialog by pressing cancel.

Using the visible prop gives the following: when set to true, the dialog never opens, and when set to false, the dialog behaves the same as omitting the visible property altogether.

When closing the modal selector manually using a ref, the 2nd duplicate callback is again invoked upon the close.

In summary, it would seem we always get the duplicate callback regardless of how the selector is closed.

Thanks.

peacechen commented 4 years ago

Thanks for the updated notes. Does your Document picker live inside or outside of the modal? It's surprising that the modal state change would cause the picker to close. Perhaps the OS native modal interferes with the native picker.

The close() method calls onModalClose(), but shouldn't call onChange() as long as you comment out the onDismiss prop on line 313. Have you tried that combination?

peacechen commented 4 years ago

@justincrow A race condition could be contributing to your corner case. As the modal is closing, it calls onChange (or onModalClose). However if the modal hasn't fully completed closing, and your Document picker starts to open, it could interfere. What may help is to delay the onModalClose callback. Since not everyone may want that delay, I propose adding a delay value prop to let users set it for their use case.

peacechen commented 4 years ago

Some historical context related to this issue: #114

AlexeyKhmelev commented 4 years ago

I found that first time it call onChange with key-label pair object {onChange: {key: "CHECK_IN", label: "Check in"}} But the second time it call it just with key {onChange: "CHECK_IN"}

so I did a weird thing:

<ModalSelector
              data={EventTypes}
              style={EVENT_TYPE_CONTAINER}
              onChange={(option) => {
                console.log({ onChange: option })
                if (option.key) { // workaround for ios https://github.com/peacechen/react-native-modal-selector/issues/140
                  setType(option.key)
                }
              }}
              selectedKey={type}
            >

And it works for me

peacechen commented 4 years ago

Thanks @AlexeyKhmelev for reporting this. Would you mind opening a new issue? It helps with tracking bug fixes.

tiendatnguyen678 commented 4 years ago
<ModalSelector
                                data={this.props.listSupporters}
                                accessible={true}
                                cancelText={'Huỷ'}
                                overlayStyle={{
                                    paddingBottom: Metrics.doubleBaseMargin,
                                    paddingTop: Metrics.screenHeight / 20
                                }}
                                cancelTextStyle={Fonts.style.normal_bold}
                                onModalClose={(option) => {
                                    option.key && this.onChangeSupporter(option)
                                }}
                                // onChange={(option) => {
                                //     this.onChangeSupporter(option)
                                //     console.log(option);
                                // }}
                                keyExtractor={(item) => item.key}>

This is my solution. Use onModalClose instead of onChage. Hope it useful!

scousino commented 4 years ago

Is there any chance this is going to be fixed?

bylucasqueiroz commented 4 years ago

I found that first time it call onChange with key-label pair object {onChange: {key: "CHECK_IN", label: "Check in"}} But the second time it call it just with key {onChange: "CHECK_IN"}

so I did a weird thing:

<ModalSelector
              data={EventTypes}
              style={EVENT_TYPE_CONTAINER}
              onChange={(option) => {
                console.log({ onChange: option })
                if (option.key) { // workaround for ios https://github.com/peacechen/react-native-modal-selector/issues/140
                  setType(option.key)
                }
              }}
              selectedKey={type}
            >

And it works for me

Thanks! It works for me.

scousino commented 3 years ago

Any update on when this is properly fixed?

peacechen commented 3 years ago

Apologies for the late response. Android and iOS native modals behave differently, and changing it for iOS could break Android. However it looks like @AlexeyKhmelev 's check for the empty key field would work.

peacechen commented 3 years ago

Fix has been published in 2.0.5. Please test this on both iOS and Android.

peacechen commented 3 years ago

good catch @glinda93 . Published 2.0.6 to address the key=0 bug.

It sounds like there are different causes for the reported issue. Have you been able to debug it?

aschmitz-wbd commented 3 years ago

The recent change does not use the keyExtractor function thus breaking use cases where the keyExtractor function is used. I just realised that onChanged is not called anymore.

peacechen commented 3 years ago

Sorry about that @aschmitz-wbd . Fix released in 2.0.7

aschmitz-wbd commented 3 years ago

Thx a lot for the quick response and fix :)

cptsponge commented 3 years ago

Hi, onChange firing twice is still happening for me on iOS 14.3, RN 0.64.2 using release 2.0.7. The onModalClose workaround as suggested by @imginnn is working

scousino commented 3 years ago

onChange is unreliable, onModalClose works without a problem

However, onModalClose does not give you the key/label info of the option you selected

Update: I see it does in the code, but the typings for this package do not indicate this. Please update your typings/docs

scousino commented 3 years ago

@scousino

I see it does in the code, but the typings for this package do not indicate this. Please update your typings/docs

maybe you can open a PR for this

166

mrtwebdesign commented 3 years ago

I just lost part of my day looking for errors in my logic due to onChange behavior being incompletely documented.

onModalClose is a more narrowly focused event and works as I expected onChange to behave. The documentation states onChange:

callback function, when the users has selected an option

It's definitely triggered by other actions and can result in a double toggle behavior

JoshuaSunsheng commented 2 years ago

Yeah, I have a same problem in my screen. It will diplay a form Modal first and then show a ModalSelector upon. After selecting a option, the onChange is called and close the ModalSelector. When I close the form Modal. The onChange of ModalSelector is called again. It is really annoyed. Changing from onChange to onModalClose does work. Thank for providing this solution.

JoshuaSunsheng commented 2 years ago

I just lost part of my day looking for errors in my logic due to onChange behavior being incompletely documented.

onModalClose is a more narrowly focused event and works as I expected onChange to behave. The documentation states onChange:

callback function, when the users has selected an option

It's definitely triggered by other actions and can result in a double toggle behavior

Thank for your solution! onModalClose works as expected.

Milutin-P commented 1 year ago

If anyone stumbles upon this issue... This is still an thing for me in v2.1.2 and react-native: 0.70.6.

Above proposed solution worked for me. I'm using onModalClose instead of onChange.

jmada commented 11 months ago

I found that first time it call onChange with key-label pair object {onChange: {key: "CHECK_IN", label: "Check in"}} But the second time it call it just with key {onChange: "CHECK_IN"}

so I did a weird thing:

<ModalSelector
              data={EventTypes}
              style={EVENT_TYPE_CONTAINER}
              onChange={(option) => {
                console.log({ onChange: option })
                if (option.key) { // workaround for ios https://github.com/peacechen/react-native-modal-selector/issues/140
                  setType(option.key)
                }
              }}
              selectedKey={type}
            >

And it works for me

Thank you so much, this works!