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 full screen loading indicator when routes are loaded lazy (or components uses React.Suspense) #9297

Merged
merged 1 commit into from Jun 28, 2021

Conversation

jerolimov
Copy link
Member

@jerolimov jerolimov commented Jun 18, 2021

Fixes:
https://issues.redhat.com/browse/ODC-5633

Analysis / Root cause:
When switching on the Pod detail page to the "Terminal" tab a lazy route / AsyncComponent was loaded and the user saw for a moment (depending on your internet speed) a strange full screen loading indicator.

This flicking happen because this loading uses react suspense and we have only one <React.Suspense /> at the application root level (app.jsx). This catch components works similar to ErrorBoundaries and could be used on different application levels.

Solution Description:
Wrap the page content and horizontal navigation tab content also with a <React.Suspense /> component.

This ensures that AsyncComponent loading on this levels shows a better loading indicator only in the content area. This would work also for other suspense usages.

I tested this by adding temporary suspense component to PodExecLoader (which us used in the terminal tab):

// Use global variables and delays only once (when rendering Delay the first time)!
let resolved: boolean = false;
let promise: Promise<null>;

const Delay = () => {
  if (!promise) {
    promise = new Promise((resolve) => {
      setTimeout(() => {
        resolved = true;
        resolve(null);
      }, 2000);
    });
  }
  if (!resolved) {
    throw promise;
  }
  return <div>Delay done!</div>;
};

// Add <Delay /> to PodExecLoader

Screen shots / Gifs for design review:

The original issue force with throttled internet connection:

original-issue

Added the Delay component above to PodExecLoader without any other change:

with-2s-suspense-call-no-other-change

When wrapping the page content (react router Switch) just in app-contents.tsx:

with-2s-suspense-call-and-catch-in-app-contents

This helps for all pages without horizontal nav. But the navigation goes away and come back after the suspense promise is resolved.

When wrapping also the tab content (react router Switch again) in horizontal-nav.tsx:

with-2s-suspense-call-and-catch-in-horizontal-nav

This fixes the issue that the tab navigation goes way and come back after the suspense promise is resolved.

Unit test coverage report:
Unchanged

Test setup:

  • Deploy and application
  • Open the Pod detail page
  • Reload your browser tab so that the Terminal component is not available!
  • Switch to the Terminal tab

Better testable on a cluster or with throttled internet connection.

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

@openshift-ci openshift-ci bot added component/core Related to console core functionality approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 18, 2021
@jerolimov
Copy link
Member Author

/assign @invincibleJai

@jerolimov
Copy link
Member Author

/retest

1 similar comment
@jerolimov
Copy link
Member Author

/retest

to ensure that any react suspense call (incl. AsyncComponent loading
doesn't show a full screen loading indicator.
@invincibleJai
Copy link
Member

/lgtm

verified the changes works as expected

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 28, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 28, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: invincibleJai, jerolimov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 3e27e1f into openshift:master Jun 28, 2021
@spadgett spadgett added this to the v4.9 milestone Jul 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants