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: can't control Modal with router params #29272

Closed
3 tasks done
legendar opened this issue Apr 4, 2024 · 6 comments
Closed
3 tasks done

bug: can't control Modal with router params #29272

legendar opened this issue Apr 4, 2024 · 6 comments
Labels

Comments

@legendar
Copy link

legendar commented Apr 4, 2024

Prerequisites

Ionic Framework Version

v7.x

Current Behavior

When the router params are changed, a new view instance is created for the parameterized route. This means that we can't re-update content (modal in our case) based on route changes.

We are using modals to show some additional details for items in the list. We also want to change the route path so the user can share the link to the particular item. This is a specific case; however, the issue is not with the modal itself but with the parameterized router.

Expected Behavior

When the router params are changed, the same view instance should be rendered but with new route params.

Steps to Reproduce

In reproduction repo:
0. Please use the modal-routing-issue branch.

  1. Open the /items/ page.
  2. Click on any item -> the route path changes to /items/:id/, and a modal with details will appear.
  3. Click on the close button -> the modal is not dismissed, although the route path changes.
  4. Click outside of the modal -> the modal will disappear due to internal state.
  5. Click on the same item -> the modal will not open. There's no way to reopen that modal anymore.

to see the expected behaviour please downgrade Ionic to version before 7.5.8:

yarn add @ionic/react@7.5.7 @ionic/react-router@7.5.7

Code Reproduction URL

https://github.com/legendar/ionic-issues/tree/modal-routing-issue

Ionic Info

Ionic:

   Ionic CLI       : 7.2.0
   Ionic Framework : @ionic/react 7.8.2

Capacitor:

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

Utility:

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

System:

   NodeJS : v18.15.0 (/home/***/.nvm/versions/node/v18.15.0/bin/node)
   npm    : 9.5.0
   OS     : Linux 5.15

Additional Information

This worked well before v7.5.8. The issue was introduced by this commit, which aimed to fix the parameterized routes issue, but also introduced this breaking change.

Video demonstrating the behavior before version 7.5.8:

before.mp4

Video demonstrating the behavior after version 7.5.8:

after.mp4
@legendar
Copy link
Author

legendar commented Apr 4, 2024

Another scenario is if we have some filtering controls on the page that also change the URL path (to allow the user to share the URL), every filter change will now (after version 7.5.8) create a new view and reset the entire page state. So, I think the solution introduced in 1705d06 is not optimal as it introduces a lot of breaking changes.

@amandaejohnston
Copy link
Contributor

Thank you for the issue. The original behavior that was changed in 1705d06 was a bug in Ionic Framework. Re-using the same view instance when navigating from one route to another is not a desired behavior; you can read more about the problems that arise in the original issue here: #26524 It sounds like your original solutions were relying on this bug in Ionic, so they will need updating now that the bug is fixed.

I can reproduce the behavior you've described in your linked reproduction, but the issue is due to the code not properly reacting to changes in the route parameters or updating isDetailsModalOpen accordingly. I was able to get everything working with the following changes:

+ import { useEffect, useState } from 'react';

- const {itemId} = useParams<{itemId: string}>();
- const isDetailsModalOpen = Boolean(itemId);
+ const params = useParams<{itemId: string}>();
+ const [isDetailsModalOpen, setDetailsModalOpen] = useState(false);

const closeDetails = () => {
  push("/items/", "none", "pop");
+ setDetailsModalOpen(false);
};

+ useEffect(() => {
+   setDetailsModalOpen(Boolean(params.itemId));
+ }, [params]);

// in the modal render
- <IonTitle>Item {itemId} details</IonTitle>
+ <IonTitle>Item {params.itemId} details</IonTitle>

Can you give this a try and let me know if you have any questions?

@amandaejohnston amandaejohnston added the needs: reply the issue needs a response from the user label Apr 9, 2024
@ionitron-bot ionitron-bot bot removed the triage label Apr 9, 2024
@legendar
Copy link
Author

Thank you for the reply. Your solution works well in this particular case👍

However, it's not a solution but rather a workaround. It relies on the fact that params is a new object for every component render, and React uses Object.is to compare dependencies in useEffect. So useEffect will always be triggered in this case. But if you try to change the code slightly to make it more prettier, it stops working☹️:

- const params = useParams<{itemId: string}>();
+ const {itemId} = useParams<{itemId: string}>();

- useEffect(() => {
-   setDetailsModalOpen(Boolean(params.itemId));
- }, [params]);
+ useEffect(() => {
+   setDetailsModalOpen(Boolean(params.itemId));
+ }, [itemId]);

The issue is much bigger than just modal showing. It affects the entire component's state and makes the React state useless. For example, if we have a checkbox in a list, there is no simple way to pass this state into the modal without additional setState, which is not the React way. See the example:

const [items, setItems] = useState<number[]>([]);

const toggleItem = (e) => {
    e.stopPropagation();

    const id = e.target.dataset.id;

    if(e.target.checked) {
      setItems(items => [...items, id])
    } else {
      setItems(items => items.filter(i => i !== id))
    }
  }

then render the list of items with checkboxes:

<IonLabel><input type="checkbox" data-id={1} onClick={toggleItem} /> Item 1</IonLabel>

and see what happens:

state.mp4

Sure, we can find a workaround for this case as well, but it's not a solution. The actual issue is that Ionic creates a new view instance every time the route is changed. So, after clicking on an item in the list, the modal doesn't react to the parameters because it's another modal in another view instance. You can verify this by changing the push call to just push("/items/") instead of push("/items/", "none", "pop") ("none" added to suppress animation).

Basically, if I specify a parameterized route, I expect the same view to be re-rendered with a new parameter and this is how react works.

<Route path="/items/past/" exact={true}>
  <ItemsList filter="past" /> {/* <-- separate view because in a separate Route component */}
</Route>
<Route path="/items/upcoming/" exact={true}>
  <ItemsList filter="upcoming" /> {/* <-- separate view because in a separate Route component */}
</Route>
<Route path="/items/:itemId?/">
  <ItemsList /> {/* <-- always the same view */}
</Route>

Yes, I understand that in some situations it's good to have a new view item for animations, etc., as shown in #26524 or in any cases where the new route corresponds to a new page. However, in other scenarios like the one I described above, where we have one big page with some internal states or filtering, it's not a good idea.
So, there should probably be a mechanism to switch between the two behaviors instead of pushing one that forces us to find a lot of workarounds for other scenarios.

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Apr 11, 2024
@liamdebeasi
Copy link
Contributor

The actual issue is that Ionic creates a new view instance every time the route is changed.

This is the intended behavior, though it differs slightly from how web applications work. Normally in web apps the JS Framework will reuse the component instance, as you noted. For example, if /page/:id maps to Foo, going from /page/1 to /page/2 will re-use the same instance of Foo. For web apps, this is fine. For mobile apps it causes issues due to how stack based navigation works.

As an example, say you are on /page/1 and you enter your email into a text input. You press a button that brings you to /page/2. Typically in a mobile app, the view for /page/2 should be separate from /page/1. Additionally, you should be able to swipe back from /page/2 to /page/1 and still see your email entered on the first view. If we reused the view instance that native mobile paradigm breaks. More specifically, you would not be able to swipe back to /page/1, and the email you entered into the text input would be missing.


I would advise against coupling modals to routing like this. Modals are temporary dialogs that are dependent on the context in which they are presented. By coupling the modal to the global state of your app (the URL), you limit how and when you can show this dialog. A lot of the complexity here could be avoided by pushing a new view with a route instead of trying to couple the modal to the route.

Part of the issue here is how views are being pushed in order to get around the modal/routing behavior. As I noted, I would recommend avoiding this entirely. But if you need to do it this way, here are some adjustments you can make to your current component. I added some documentation to explain some of the changes I made.

import { IonContent, IonHeader, IonPage, IonTitle, IonToolbar, IonList, IonItem, IonLabel, IonModal, useIonRouter, IonButtons, IonButton, createAnimation } from '@ionic/react';
import { useParams } from "react-router";

/**
  Setting routerDirection="none" seems to cause new views to be mounted
  and then never unmounted which is contributing to the problem here. To
  avoid this, we pass in a blank animation to skip the transition effect.
  Seems related to https://github.com/ionic-team/ionic-framework/issues/24074
*/
const animation = () => createAnimation();

export default function ItemsList() {
  const { itemId } = useParams<{itemId: string}>();

  /**
   * Note that since you are navigating backwards without an animation, the view
   * and therefore the modal will be unmounted immediately. As a result, the modal
   * will not have a dismissing animation. This is one of the reasons why it's best
   * to avoid coupling a modal with routing.
   */
  const router = useIonRouter();
  const closeDetails = () => {
    router.goBack();
  };

  return (
    <>
      <IonPage>
        <IonHeader>
          <IonToolbar>
            <IonTitle>Items list. Click on item to open details</IonTitle>
          </IonToolbar>
        </IonHeader>
        <IonContent>
          <IonList>
            <IonItem
              button
              routerLink={`/items/1/`}
              routerAnimation={animation}
              detail={true}
            >
              <IonLabel>Item 1</IonLabel>
            </IonItem>
            <IonItem
              button
              routerLink={`/items/2/`}
              routerAnimation={animation}
              detail={true}
            >
              <IonLabel>Item 2</IonLabel>
            </IonItem>
          </IonList>
        </IonContent>
      </IonPage>
      <IonModal
        isOpen={itemId !== undefined}
        onWillDismiss={closeDetails}
        initialBreakpoint={0.9}
        breakpoints={[0.9]}
      >
        <IonHeader>
          <IonToolbar>
            <IonButtons slot="start">
              <IonButton onClick={closeDetails}>Close</IonButton>
            </IonButtons>
            <IonTitle>Item {itemId} details</IonTitle>
          </IonToolbar>
        </IonHeader>
        <IonContent>
          <p>
            Lorem Ipsum is simply dummy text of the printing and typesetting industry. Lorem Ipsum has been the industry's standard dummy text ever since the 1500s, when an unknown printer took a galley of type and scrambled it to make a type specimen book. It has survived not only five centuries, but also the leap into electronic typesetting, remaining essentially unchanged. It was popularised in the 1960s with the release of Letraset sheets containing Lorem Ipsum passages, and more recently with desktop publishing software like Aldus PageMaker including versions of Lorem Ipsum
          </p>
        </IonContent>
      </IonModal>
    </>
  );
}

As I noted previously, the current behavior in Ionic is intended, so I am going to close this.

@liamdebeasi liamdebeasi closed this as not planned Won't fix, can't repro, duplicate, stale Apr 19, 2024
@liamdebeasi liamdebeasi removed their assignment Apr 19, 2024
@legendar
Copy link
Author

@liamdebeasi Thanks for the reply.

I understand why this is considered intended behavior. However, I can't agree that it should be the only behavior. The example with the modal is not very illustrative, but as I mentioned earlier, the issue is not specific to modals but affects anything that depends on routing changes.

Here is another example with content filtering. We have a custom filtering element at the top of the page, and underneath that, there's a list that filters based on the choices made in this element. See the screenshot of one of the filtering elements below:

image

When a user makes a choice, we want to display the same view and just filter the content. However, we also want to change the route so the user can share a link or open the page with a predefined value for this selector. Additionally, we want the swipe action to navigate to the previous view (any other page), but not just change the value in the selector. This is easy to achieve by disabling page animation and using the replace action on the route.
We also want to display a bubble with a hint for some choices, and we want the arrow of the bubble to be aligned with the selected choice. The issue here is that we can't use animation for the bubble arrow, as on every route change, a new view with a new bubble element and new arrow will be created.

@legendar
Copy link
Author

legendar commented May 9, 2024

@liamdebeasi, any suggestions regarding my last note? Or should I create another ticket for this case?

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

No branches or pull requests

3 participants