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

bug: conditionally rendered modal via route doesn't close when navigating away #25775

Closed
5 of 7 tasks
corysmc opened this issue Aug 17, 2022 · 10 comments
Closed
5 of 7 tasks
Assignees
Labels
package: react @ionic/react package type: bug a confirmed bug report

Comments

@corysmc
Copy link
Contributor

corysmc commented Aug 17, 2022

Prerequisites

Ionic Framework Version

  • v4.x
  • v5.x
  • v6.x
  • Nightly

Current Behavior

After updating to ionic 6, Conditionally rendered ion-modals are not dismissing after changing routes. There's a fix in route for an error that happened when the modal was removed from the dom #25590 - but now the modal won't dismiss when navigating away, and the animations aren't happening - and the modal won't dismiss after changing routes.

A route based conditionally rendered IonModal worked in ionic v5 - and I've been told this is a common pattern in a react app, so I would expect this to work out of the box.

Video:

Here's a video of the simple demo I put together based off of a fork from the ionic-react-conference app
https://user-images.githubusercontent.com/6452188/185195963-06ad49bf-0533-41ba-9f6e-0cb01326eff9.mov

Expected Behavior

When conditionally rendering an IonModal in React I would expect to be able to show/hide an ion modal based on a matched route. Ideally it would also animate in/out when mounting and unmounting (see video for hack workaround to get this working)

Steps to Reproduce

  1. Install the dev build that fixes the original issue "Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node."
  2. Add a Modal component that has prop isOpen={true}
  3. Add a Switch & Route to the component
  4. Inside the Modal Component change the route
  5. Route to main component ("/support"), click on "New Support Ticket", then click one of the back buttons
  6. ⚠️ Notice the modal doesn't close (see video above)

Step 1
yarn add @ionic/[email protected] and yarn add @ionic/[email protected] or npm i @ionic/[email protected] and npm i @ionic/[email protected]

Step 2
The SupportModal component above looks something like this: <IonModal isOpen>...

Step 3

...
<IonContent>
  <IonButton href="/support/new">New Support Ticket</IonButton>
</IonContent>
<Switch>
  <Route path="/support/new" exact component={SupportModal} />
</Switch>
...

Step 4
Add this to the SupportModal
<IonButton href="/support">Go Back</IonButton>

Full code here: https://github.com/corysmc/ionic-react-conference-app

Code Reproduction URL

https://github.com/corysmc/ionic-react-conference-app

Ionic Info

Ionic:

Ionic CLI : 6.19.0
Ionic Framework : @ionic/react 6.2.3-dev.11660337759.18ea0f7e

Capacitor:

Capacitor CLI : 1.3.0
@capacitor/android : not installed
@capacitor/core : 1.3.0
@capacitor/ios : not installed

Utility:

cordova-res : not installed globally
native-run : not installed globally

System:

NodeJS : v14.18.2
npm : 6.14.15
OS : macOS Monterey

Additional Information

Related issues
#25590
#24887

@ionitron-bot ionitron-bot bot added the triage label Aug 17, 2022
@liamdebeasi liamdebeasi self-assigned this Aug 17, 2022
@liamdebeasi
Copy link
Contributor

Thanks for the issue. Are you able to provide a repo that contains only the minimum amount of code required to reproduce this issue? I am getting import errors when I try to run your app, possibly due to outdated dependencies.

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Aug 17, 2022
@liamdebeasi liamdebeasi removed their assignment Aug 17, 2022
@ionitron-bot ionitron-bot bot removed the triage label Aug 17, 2022
@corysmc
Copy link
Contributor Author

corysmc commented Aug 17, 2022

Hmm that's interesting , maybe because I was using yarn instead of npm.
I was able to reproduce again by cloning to my other computer, running npm install and npm start - It did change my package-lock file - so I've committed those changes. Can you try again @liamdebeasi ?
Thanks

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Aug 17, 2022
@corysmc
Copy link
Contributor Author

corysmc commented Aug 17, 2022

@liamdebeasi - here's an even smaller repo: https://github.com/corysmc/test-route-modal

@liamdebeasi
Copy link
Contributor

liamdebeasi commented Aug 17, 2022

Thanks. While coupling modals with routing is an anti-pattern and not something we encourage, conditionally rendering the modal should remove the underlying Web Component. Let me take a closer look and see why this is happening.

@corysmc
Copy link
Contributor Author

corysmc commented Aug 18, 2022

Thanks @liamdebeasi - I really appreciate it! I totally agree about the anti-pattern. I'll see if we can make the change internally to use modals properly.

For reference here's another related issue
#23567

@sean-perkins
Copy link
Contributor

Hey @corysmc can you test with this dev-build and let me know if you run into any odd quirks?

I tested with the test-route-modal repository and it appears to dismiss correctly on route actions.

@IhorHolovatsky
Copy link

@ionic/[email protected]
works for me too. The modal unmounted correctly when navigating to another page

@puopg
Copy link
Contributor

puopg commented Sep 9, 2022

Any status update on this fix?

@sean-perkins
Copy link
Contributor

@puopg this is still an active bug in discovery/development.

The issue of dismissing the overlay on route navigation has been resolved in the dev-build, but is also dependent on another bug: #25590 that requires additional effort.

We have multiple strategies of presenting and controlling the rendering of overlays (isOpen, trigger, controllers, keepContentsMounted, etc.). The challenge that I am currently working through is solving this bug in a way that does not regress other presenting strategies. Moving the rendering of the overlay to a React Portal solves the pain points of both issues for isOpen usages, but regresses trigger implementations.

I will share an update on both issues once I validate a strategy that works and is well tested. I will also likely share a new dev-build when that occurs.

Thanks!

@ionitron-bot
Copy link

ionitron-bot bot commented Apr 2, 2023

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Apr 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: react @ionic/react package type: bug a confirmed bug report
Projects
None yet
Development

No branches or pull requests

5 participants