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

iOS onChange called twice #140

Open
justincrow opened this issue Mar 16, 2020 · 34 comments
Open

iOS onChange called twice #140

justincrow opened this issue Mar 16, 2020 · 34 comments

Comments

@justincrow
Copy link

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

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
Copy link
Author

justincrow commented Mar 18, 2020

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
Copy link
Owner

peacechen commented Mar 18, 2020

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
Copy link

pr4nta commented Mar 29, 2020

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
Copy link
Owner

@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
Copy link
Owner

peacechen commented Mar 29, 2020

@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
Copy link
Author

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
Copy link
Owner

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
Copy link
Owner

@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
Copy link
Owner

Some historical context related to this issue: #114

@AlexeyKhmelev
Copy link

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
Copy link
Owner

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

@tiendatnguyen678
Copy link

tiendatnguyen678 commented Aug 10, 2020

<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
Copy link

Is there any chance this is going to be fixed?

@bylucasqueiroz
Copy link

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
Copy link

Any update on when this is properly fixed?

@peacechen
Copy link
Owner

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
Copy link
Owner

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

@peacechen
Copy link
Owner

peacechen commented Jul 7, 2021

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
Copy link

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
Copy link
Owner

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

@aschmitz-wbd
Copy link

Thx a lot for the quick response and fix :)

@cptsponge
Copy link

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
Copy link

scousino commented Jul 26, 2021

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
Copy link

@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
Copy link

mrtwebdesign commented Nov 13, 2021

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
Copy link

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
Copy link

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
Copy link

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
Copy link

jmada commented Dec 11, 2023

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

14 participants