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

fix(react): modal now mounts child component independently of other modals #23903

Merged
merged 3 commits into from
Sep 13, 2021

Conversation

puopg
Copy link
Contributor

@puopg puopg commented Sep 8, 2021

Currently, when Ion components such as IonModal are created using the createOverlayComponent HOC, the isDismissing flag is scoped to the definition of the class. So in this case, when IonModal gets exported, the isDismissing flag is scoped to the IonModal class definition and not an instance of the IonModal.

Consider when you have multiple IonModals controlling different dialogs or whatever. So I believe this isDismissing flag should be scoped to the instance of the Overlay class, as if I chose to render 10 IonModals, each should be controlled independently.

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (npm run build) was run locally and any changes were pushed
  • Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue Number: resolves #23904

What is the new behavior?

Does this introduce a breaking change?

  • Yes
  • No

Other information

Currently, when Ion components such as `IonModal` are created using the `createOverlayComponent` HOC, the isDismissing flag is scoped to the definition of the class. So in this case, when IonModal gets exported, the isDismissing flag is scoped to the IonModal class definition and not an instance of the IonModal. 

Consider when you have multiple IonModals controlling different dialogs or whatever. So I believe this `isDismissing` flag should be scoped to the instance of the Overlay class, as if I chose to render 10 IonModals, each should be controlled independently.
@github-actions github-actions bot added the package: react @ionic/react package label Sep 8, 2021
@liamdebeasi liamdebeasi changed the title Scope isDismissing to instance of Overlay fix(react): modal now mounts child component independently of other modals Sep 9, 2021
@puopg
Copy link
Contributor Author

puopg commented Sep 9, 2021

Ready to go!

@liamdebeasi
Copy link
Contributor

The code looks good, but do you have a GitHub repo that shows the issue happening? I would like to verify this fix by testing in that app.

@puopg
Copy link
Contributor Author

puopg commented Sep 10, 2021

@liamdebeasi Yea heres a code sandbox with ionic/react 5.7.0 showcasing the issue.

https://codesandbox.io/s/nervous-sky-w954i

Repo if you so desire: https://github.com/puopg/ioni-test

@puopg
Copy link
Contributor Author

puopg commented Sep 10, 2021

BEFORE:

Screen.Recording.2021-09-10.at.10.32.16.AM.mov

AFTER:

Screen.Recording.2021-09-10.at.10.31.55.AM.mov

Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job!

@liamdebeasi liamdebeasi merged commit 1e13429 into ionic-team:main Sep 13, 2021
@liamdebeasi
Copy link
Contributor

Merged. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: react @ionic/react package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: isDismissing on Overlay is scoped to exported IonModal class and not instances of the IonModal.
2 participants