Skip to content

fix(react): replacing route uses new route direction and animation - #28671

Merged
sean-perkins merged 4 commits into
ionic-team:mainfrom
aeharding:replace
Dec 11, 2023
Merged

fix(react): replacing route uses new route direction and animation#28671
sean-perkins merged 4 commits into
ionic-team:mainfrom
aeharding:replace

Conversation

@aeharding

@aeharding aeharding commented Dec 8, 2023

Copy link
Copy Markdown
Contributor

Issue number: resolves #24260


What is the current behavior?

When replacing a route (router.push(newRoute, 'none', 'replace')) the RouterDirection from the route being replaced is being used (if it exists) instead of the new one the user specifies.

What is the new behavior?

User-specified RouteDirection is used, if it exists. If it doesn't it falls back to the RouteDirection of the route being replaced.

Does this introduce a breaking change?

  • Yes
  • No

Other information

Please see the following comment for why I think the current behavior is incorrect, and why this change is needed: #24260 (comment)

@github-actions github-actions Bot added package: core @ionic/core package package: react @ionic/react package labels Dec 8, 2023
@github-actions github-actions Bot removed the package: core @ionic/core package label Dec 8, 2023
Comment thread packages/react-router/src/ReactRouter/IonRouter.tsx Outdated
@liamdebeasi

Copy link
Copy Markdown
Contributor

Thanks! Some more context for why this bug exists. When we are replacing, we need to copy over some of the previous state. For example, consider a history of A --> B. When replacing to C, we are replacing the state of B with the state of C. As result, the previous route for C is A (since B no longer exists).

Due to this requirement, we correctly copy information from currentRouteInfo first before falling back to routeInfo:

const currentRouteInfo = this.locationHistory.current();
/**
* If going from /home to /child, then replacing from
* /child to /home, we don't want the route info to
* say that /home was pushed by /home which is not correct.
*/
const currentPushedBy = currentRouteInfo?.pushedByRoute;
const pushedByRoute =
currentPushedBy !== undefined && currentPushedBy !== routeInfo.pathname
? currentPushedBy
: routeInfo.pushedByRoute;
routeInfo.lastPathname = currentRouteInfo?.pathname || routeInfo.lastPathname;
routeInfo.prevRouteLastPathname = currentRouteInfo?.lastPathname;
routeInfo.pushedByRoute = pushedByRoute;

However, we should not do that for routeDirection and routeAnimation. The reason is that developers may have specified a custom animation or direction to use with this replace operation. We should prefer that first, and then fall back to the data in currentRouteInfo if the developer does not pass any information.

Comment thread packages/react-router/src/ReactRouter/IonRouter.tsx Outdated
Co-authored-by: Liam DeBeasi <liamdebeasi@users.noreply.github.com>
@aeharding

Copy link
Copy Markdown
Contributor Author

Thanks @liamdebeasi, updated!

@aeharding
aeharding requested a review from liamdebeasi December 8, 2023 14:36

@liamdebeasi liamdebeasi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good to go once the final change is made. I'm making a dev build now and will post this in the linked thread.

Comment thread packages/react-router/src/ReactRouter/IonRouter.tsx
Co-authored-by: Liam DeBeasi <liamdebeasi@users.noreply.github.com>
@aeharding aeharding changed the title fix(react): replacing route uses new route direction fix(react): replacing route uses new route direction and animation Dec 8, 2023
github-merge-queue Bot pushed a commit that referenced this pull request Dec 11, 2023
)

Issue number: resolves #28673

---------

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->

When reviewing #28671
I noticed a bug where the custom animation was not used when going back
after a replace.

`handleNavigate` will override whatever is in `incomingRouteParams`.
Since we were passing `routeAnimation` (which is `undefined`), it was
overriding the animation we set in `handleNavigateBack`.

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- `routeAnimation` is no longer overridden

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!-- If this introduces a breaking change, please describe the impact
and migration path for existing applications below. -->


## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->

Dev build: `7.6.1-dev.11702048520.13c82dad`

@thetaPC thetaPC left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, works as intended.

@sean-perkins sean-perkins left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for fixing this!

@sean-perkins
sean-perkins added this pull request to the merge queue Dec 11, 2023
Merged via the queue into ionic-team:main with commit a17b963 Dec 11, 2023
@aeharding
aeharding deleted the replace branch December 11, 2023 21:32
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: Ionic React navigate with push from useIonRouter ignores direction when used as "replace"

4 participants