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-router): remove page transition flicker on outlet mounting #24667

Merged
merged 3 commits into from
Jan 27, 2022

Conversation

sean-perkins
Copy link
Contributor

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?

On initial load, when using nested router outlets with React, another outlet can briefly display before the enter view & outlet has mounted.

Issue Number: #24666

What is the new behavior?

  • Page transitions that are deferred as a result of the outlet not being rendered yet, happen immediately after the outlet is mounted.
  • Add note about using a deprecated private API from react-router (computedMatch) that is no longer available in v6.
  • Removes tabs attribute when cloning the page element
    • Cannot find a single reference to why we have this code. Looks like carry over from the Angular router integration, which also has no functional code tied to it.

Does this introduce a breaking change?

  • Yes
  • No

Other information

Before After
Kapture.2022-01-26.at.19.45.15.mp4
Kapture.2022-01-26.at.20.01.10.mp4

Note: I am refreshing the browser in both examples.

@sean-perkins sean-perkins requested a review from a team as a code owner January 27, 2022 01:16
@github-actions github-actions bot added the package: react @ionic/react package label Jan 27, 2022
*/
this.pendingPageTransition = true;

this.setState({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This may be redundant. Open to feedback for those that are more familiar with how React handles internal state changes.

Choose a reason for hiding this comment

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

@sean-perkins - are you tossing routeInfo into state as a way to get another render so that pendingPageTransition can be evaluated? Would it be clearer to put pendingPageTransition into state (instead of an instance variable) and use it from state?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to maintain as much previous code behavior, of passing the context of routeInfo from the original function call, back when the function should be handled.

Although, I don't believe routeInfo can change between the time React component is first constructed and the first render cycle. So I believe the setState is just re-assigning the same value and can be removed. I did brief testing of removing that call and tests passed and behavior seemed appropriate.

The important piece is just catching when a page transition happens on a component that hasn't mounted (typically from routing directly to a view in a nested outlet) and re-handling that when the component is available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't believe componentOnReady will be applicable here. The most common state of this happening, was when routerOutlet (the element ref) was undefined.

From testing, it does not appear to be needed in the componentDidUpdate life cycle 👍

@sean-perkins sean-perkins merged commit bdb5c42 into main Jan 27, 2022
@sean-perkins sean-perkins deleted the FW-644 branch January 27, 2022 21:29
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.

4 participants