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

Change progress bar to spinner #78460

Merged
merged 10 commits into from Sep 28, 2020
Merged

Conversation

ryankeairns
Copy link
Contributor

@ryankeairns ryankeairns commented Sep 24, 2020

Summary

With the implementation of navigational search, the global progress bar became even more noticeable due to the frequent loading nature of the typeahead interaction. In other words, the pink progress bar flashes constantly as you type and it's rather distracting. This PR converts the current loading bar to a spinner that appears within the new stacked header (alongside the logo).

Before

oldLoading

After

newdesign

Keeping the progress bar option for full screen mode

Same look as before but with EuiProgressBar

fullscreenLoading

Related changes in this PR

Single loading indicator on initial screen

A nice side effect of moving the loading indicator into the header is that we avoid having two loading elements on the initial loading screen. Notice the absence of the pink progress bar below:

main-loading

Animated cluster between apps

Beyond the initial loading screen, an additional spinner (not the Elastic logo) could occasionally be seen in the body of the page. To avoid having the same spinner within the header and the page, I've changed the page spinner to use the animated cluster (EuiLoadingElastic). This one appears less frequently due to the new platform (thankfully), but here's a glimpse:

animatedCluster

Minimize loading 'flashes'

Lastly, I have also tuned the debounce setting to further reduce the number of spinner flashes seen as you type in the search input. Compared to the 'Before' gif atop this issue, you'll see the results panel no longer flickers as well.

spinnerDebounce


Implementation notes

  • I chose to use an aria-hidden: true approach - coupled with visibility: hidden - in order to preserve the real estate used by the spinner in the header. This prevents the search input from jumping around as it would if the spinner was hidden via display: none , for example.
  • The spinner continues to animate even while hidden, so I have also added a animation-play-state: paused rule. I couldn't live with this, even if it's not visible:
    pauseAnimation

Checklist

Delete any items that are not applicable to this PR.

@ryankeairns
Copy link
Contributor Author

jenkins test this

@ryankeairns ryankeairns added REASSIGN from Team:Core UI Deprecated label for old Core UI team Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. v7.10.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Sep 28, 2020
@ryankeairns ryankeairns marked this pull request as ready for review September 28, 2020 13:29
@ryankeairns ryankeairns requested review from a team as code owners September 28, 2020 13:29
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core-ui (Team:Core UI)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-design (Team:Kibana-Design)

@ryankeairns ryankeairns requested a review from a team September 28, 2020 13:36
@pgayvallet
Copy link
Contributor

@ryankeairns is the interaction with the searchbar the main reason for this change? If so, we could add an option to http.fetch to not show the loader for a specific request.

(If it was also planned for other reasons, just forget about the proposition)

@ryankeairns
Copy link
Contributor Author

@ryankeairns is the interaction with the searchbar the main reason for this change? If so, we could add an option to http.fetch to not show the loader for a specific request.

(If it was also planned for other reasons, just forget about the proposition)

@pgayvallet Thanks for the option. There has been clamoring (on the design side, at least) to improve the pink progress bar for some time and, yes, the new search feature made it even more visible. The loading indicator is useful for search, imo, so I've opted to try this new approach.

@ryankeairns
Copy link
Contributor Author

jenkins test this

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

page load bundle size

id value diff baseline
core 1.1MB -2.8KB 1.1MB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Pretty elegant solution! Quick question, how will this now work when we eventually have either an OSS or deployment badge next to the Elastic brand? Are you good with the extra space that would create? Or are you thinking that we're now restricting additional header elements to the right side only?

Screen Shot 2020-09-28 at 16 43 51 PM

Screen Shot 2020-09-28 at 16 46 39 PM

@ryankeairns
Copy link
Contributor Author

Pretty elegant solution! Quick question, how will this now work when we eventually have either an OSS or deployment badge next to the Elastic brand? Are you good with the extra space that would create? Or are you thinking that we're now restricting additional header elements to the right side only?

Screen Shot 2020-09-28 at 16 43 51 PM Screen Shot 2020-09-28 at 16 46 39 PM

Thanks for the review!

My thought was that the deployment switcher winds up on the right side. It's been presented both ways and, frankly, I'm not sure when this will get added at the current pace.

We have some flexible options for the OSS badge. For example, we could flip back to the progress bar (via the new showAsBar prop) since it does not have search. If we end up showing badges beyond OSS, then we may have to play with the layout.

There is a third consideration here, too, and that is the async stuff that DeFazio is working on in the breadcrumb area. We chatted and can/will avoid displaying two loading spinners.

Overall, this PR is a low investment refactoring of the current loading indicator code that could be easily be changed if/when those other things come along in the future.

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

I 100% agree this is a much needed upgrade. Just wanted to know if you'd thought out that badge placement or if that is a thing of the past. We tend to shift content around on the user a lot and wanted to see if we could avoid that here. I'd probably resist changing back to the progress bar just for OSS, seems like an odd distinction/difference to make. But in that case the extra space is probably fine. 👍 😍

@ryankeairns ryankeairns merged commit 3094ca1 into elastic:master Sep 28, 2020
ryankeairns added a commit to ryankeairns/kibana that referenced this pull request Sep 28, 2020
* Change progress bar to spinner

* Add progress bar option for full screen mode

* Update snapshots for router test

* Update snapshots for loading indicator test

* Update header snapshot

* Change progress bar position to fix full screen
ryankeairns added a commit that referenced this pull request Sep 28, 2020
* Change progress bar to spinner

* Add progress bar option for full screen mode

* Update snapshots for router test

* Update snapshots for loading indicator test

* Update header snapshot

* Change progress bar position to fix full screen
phillipb added a commit to phillipb/kibana that referenced this pull request Sep 29, 2020
…a into add-anomalies-to-timeline

* 'add-anomalies-to-timeline' of github.com:phillipb/kibana: (89 commits)
  Aligns several module versions across the repository (elastic#78327)
  Empty prompt and loading spinner for service map (elastic#78382)
  Change progress bar to spinner (elastic#78460)
  [QA][Code Coverage] Coverage teams lookup w/o Additional Config (elastic#77111)
  Slim down core bundle (elastic#75912)
  [Alerting] retry internal OCC calls within alertsClient (elastic#77838)
  [kbn/optimizer] only build xpack examples when building xpack plugins (elastic#78656)
  [Ingest Manager] Ingest setup upgrade (elastic#78081)
  [Ingest Manager] Surface saved object client 10,000 limitation to bulk actions UI (elastic#78520)
  fix name without a category or if field end with .text (elastic#78655)
  [Security Solution] [Detections] Log message enhancements (elastic#78429)
  [ML]DF Analytics exploration: default filter of results page by `defaultIsTraining` value in url (elastic#78303)
  [Enterprise Search] Remove all instances of KibanaContext to Kea store (elastic#78513)
  [ML] DF Analytics creation: ensure job did not fail to start before showing results link (elastic#78200)
  fix createAppNavigationHandler to use `navigateToUrl` (elastic#78583)
  Fixing a11y test failure on discover app (elastic#59975) (elastic#77614)
  [Security Solution] Initiate endpoint package upgrade from security app (elastic#77498)
  [kbn/es] use a basic build process (elastic#78090)
  [kbn/optimizer] fix .json extension handling (elastic#78524)
  Fix APM lodash imports (elastic#78438)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Sep 29, 2020
* master: (365 commits)
  making expression debug info serializable (elastic#78727)
  fix lodahs imports in app-arch code (elastic#78582)
  Make Field a React.lazy export (elastic#78483)
  [Security Solution] Improves detections tests (elastic#77295)
  [TSVB] Different field format on different series is ignored (elastic#78138)
  RFC: Improve saved object migrations (elastic#66056)
  [Security Solution] Fixes url timeline flaky test (elastic#78556)
  adds retryability feature (elastic#78611)
  Aligns several module versions across the repository (elastic#78327)
  Empty prompt and loading spinner for service map (elastic#78382)
  Change progress bar to spinner (elastic#78460)
  [QA][Code Coverage] Coverage teams lookup w/o Additional Config (elastic#77111)
  Slim down core bundle (elastic#75912)
  [Alerting] retry internal OCC calls within alertsClient (elastic#77838)
  [kbn/optimizer] only build xpack examples when building xpack plugins (elastic#78656)
  [Ingest Manager] Ingest setup upgrade (elastic#78081)
  [Ingest Manager] Surface saved object client 10,000 limitation to bulk actions UI (elastic#78520)
  fix name without a category or if field end with .text (elastic#78655)
  [Security Solution] [Detections] Log message enhancements (elastic#78429)
  [ML]DF Analytics exploration: default filter of results page by `defaultIsTraining` value in url (elastic#78303)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
REASSIGN from Team:Core UI Deprecated label for old Core UI team release_note:skip Skip the PR/issue when compiling release notes Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants