Moving loader to logo in header, add a slight 250ms pause by snide · Pull Request #78879 · elastic/kibana

snide · 2020-09-29T22:07:35Z

Summary

This adds on top of @ryankeairns work in #78460

It makes two changes:

Moves the spinner to live in place of the logo when active
Delays the spinner to only show after 250ms to prevent unnecessary loading events
Changes the "Elastic" wording to be an svg of our mark.

Hat tip to @thompsongl for helping me through the pause mechanics.

Demo

https://snid.es/2020SEP/56iaFdx9gOw7gwua.mp4

Note that moving to the dashboard list no longer shows the loader. Before it would "blip" on these small micro-loads.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

src/core/public/chrome/ui/header/header_logo.tsx

- Removed inline fills for CSS implementation - Use the `euiHeaderLogo` class and remove custom styles

pgayvallet · 2020-09-30T05:57:45Z

src/core/public/chrome/ui/loading_indicator.tsx

    visible: false,
  };

+  private timer: any;


NIT: private timer?: number;

I don't think this is a number. It relates to the setTimeout function. TS complains.

timeout ids are numbers

setTimeout(handler: TimerHandler, timeout?: number, ...arguments: any[]): number; clearTimeout(handle?: number): void;

What does TS complains about exactly?

pgayvallet · 2020-09-30T06:01:08Z

src/core/public/chrome/ui/loading_indicator.tsx

+      if (this.increment > 1) {
+        clearTimeout(this.timer);
+      }
+      this.increment += this.increment;


As increment is initialized to 1, I'm ensure to understand what exactly this block does. Both the if (this.increment > 1) (that would be always true) and this.increment += this.increment; seems wrong to me?

It makes sure that the 250 pause only starts the first time the cycle begins. You can see that it does print initially and only increases when the subscription does.

The subscription code isn't the most approachable, but this method seemed to do what was needed: only show a loading indicator if a new "loading event" existed for more than 250ms.

pgayvallet · 2020-09-30T06:04:45Z

src/core/public/chrome/ui/loading_indicator.tsx

+      if (this.increment > 1) {
+        clearTimeout(this.timer);
+      }
+      this.increment += this.increment;
+      this.timer = setTimeout(() => {
+        this.setState({
+          visible: count > 0,
+        });
+      }, 250);


So, the delay is reset every time loadingCount emits a new value, meaning that if there are consecutive requests firing at an interval < 250, the loader will never be displayed until 250ms after the last one. Is that the expected behavior?

Shouldn't we start the timer the first time loadingCount$ is greater than 0, but not on the next emissions until loadingCount is back to 0?

I'll be honest, if you have experience with this code, feel free to commit on top of it. The way the subscription numbers shifted felt weird.

In any case the problem I was trying to solve from a UI perspective with the current functionality:

The loading action "blips" for minor events. Basically, ANY time the subscription changed (which is often) it would signal. Less than a second of loading, and the user won't even notice the load, but they would notice an annoying animation on the page. We'd prefer not to show it in those cases, and only show it for sustained loads. I think what your'e saying is the correct way to handle it. Look for it to go back to zero, then start the count, and see if the number is larger than zero.

+1 to Pierre's comments

I can add this to my queue if you're not sure how to implement it but I don't think I'll get to it for at least a few days (if someone else has time before me, feel free to jump in)

I'm gonna give this a shot tonight. I'll bug you all tomorrow for a review.

src/core/public/chrome/ui/header/elastic_mark.tsx

mshustov · 2020-09-30T06:18:17Z

src/core/public/chrome/ui/loading_indicator.test.tsx

+    // Pause the check beyond the 250ms delay that it has
+    setTimeout(() => {
+      expect(wrapper.prop('data-test-subj')).toBe('globalLoadingIndicator');
+    }, 300);


optional: It could be a source of flakiness on CI. We might implement a function polling the element presence (as in FTR TestSubjects.exists or remove the time factor in tests https://jestjs.io/docs/en/timer-mocks#run-all-timers

src/core/public/chrome/ui/loading_indicator.tsx

ryankeairns

This looks and works nicely, thanks for putting it together.

I see others have reviewed and left comments on the code. I also took a look, based upon what I learned in the preceding PR, and things look good.

cchaos · 2020-10-01T15:01:36Z

src/core/public/chrome/ui/header/elastic_mark.tsx

+    <title id="elasticMark">Elastic</title>
+    <path
+      d="M9.74 16.882l.711-.076.046 1.455c-1.879.257-3.485.393-4.818.393-1.773 0-3.03-.515-3.773-1.545C1.164 16.08.8 14.473.8 12.306c0-4.333 1.727-6.5 5.167-6.5 1.666 0 2.909.47 3.727 1.394.818.924 1.227 2.394 1.227 4.379l-.106 1.409H2.664c0 1.364.242 2.379.742 3.03.5.652 1.349.985 2.576.985 1.242.03 2.485-.015 3.757-.121zm-.667-5.349c0-1.515-.243-2.59-.728-3.212-.484-.621-1.272-.94-2.363-.94s-1.924.334-2.47.986c-.545.651-.833 1.712-.848 3.166h6.409zM13.497 18.427V.7h1.848v17.727h-1.848zM27.027 9.806v6.076c0 .621 1.53.742 1.53.742l-.09 1.637c-1.303 0-2.38.106-3.03-.515a10.861 10.861 0 01-4.44.924c-1.136 0-2-.319-2.59-.97-.592-.636-.895-1.56-.895-2.773 0-1.197.303-2.09.91-2.651.605-.56 1.56-.925 2.863-1.046l3.879-.363v-1.06c0-.834-.182-1.44-.546-1.804-.363-.364-.863-.545-1.485-.545H18.27V5.82h4.742c1.394 0 2.41.318 3.046.97.651.636.97 1.651.97 3.015zm-7.606 5.03c0 1.516.621 2.273 1.879 2.273a9.89 9.89 0 003.303-.56l.56-.197v-4.076l-3.65.348c-.743.06-1.274.273-1.607.637-.333.363-.485.894-.485 1.575zM34.255 7.473c-1.788 0-2.697.62-2.697 1.879 0 .575.212.984.62 1.227.41.242 1.35.485 2.819.742 1.47.258 2.5.606 3.106 1.076.606.454.91 1.318.91 2.59 0 1.274-.41 2.198-1.228 2.789-.818.59-2 .894-3.576.894-1.015 0-4.424-.38-4.424-.38l.106-1.605c1.954.182 3.379.333 4.333.333.955 0 1.682-.151 2.182-.454.5-.303.758-.819.758-1.53 0-.713-.212-1.198-.637-1.455-.424-.258-1.363-.5-2.818-.728-1.455-.227-2.485-.56-3.09-1.015-.607-.439-.91-1.272-.91-2.47 0-1.196.424-2.09 1.273-2.666.848-.576 1.909-.864 3.166-.864 1 0 4.485.258 4.485.258v1.621c-1.833-.106-3.333-.242-4.379-.242zM47.952 7.685h-3.925v5.909c0 1.409.106 2.348.303 2.788.212.44.697.666 1.47.666l2.197-.151.121 1.53c-1.106.182-1.94.273-2.515.273-1.288 0-2.167-.318-2.667-.94-.5-.62-.742-1.818-.742-3.575v-6.5h-1.758V6.079h1.758V2.29h1.833v3.773h3.925v1.62zM50.527 3.276V1.139h1.849v2.152l-1.849-.015zm0 15.151V6.08h1.849v12.348h-1.849zM60.406 5.821c.546 0 1.47.106 2.773.303l.59.076-.075 1.5c-1.318-.152-2.288-.227-2.91-.227-1.393 0-2.348.333-2.848 1-.5.666-.757 1.909-.757 3.712 0 1.803.227 3.06.697 3.773.47.712 1.44 1.06 2.924 1.06l2.91-.227.075 1.53c-1.53.227-2.682.349-3.44.349-1.924 0-3.257-.5-3.984-1.485-.728-.985-1.106-2.652-1.106-5 0-2.349.393-4 1.181-4.94.803-.939 2.122-1.424 3.97-1.424z"
+      fill="#fff"


You've got the fill back in here. I'd remove it.

@snide I'd remove this fill. Then I think it's GTG

src/core/public/chrome/ui/header/header_logo.tsx

myasonik · 2020-10-01T15:19:35Z

src/core/public/chrome/ui/loading_indicator.tsx

+      if (this.increment > 1) {
+        clearTimeout(this.timer);
+      }
+      this.increment += this.increment;
+      this.timer = setTimeout(() => {
+        this.setState({
+          visible: count > 0,
+        });
+      }, 250);


+1 to Pierre's comments

I can add this to my queue if you're not sure how to implement it but I don't think I'll get to it for at least a few days (if someone else has time before me, feel free to jump in)

snide · 2020-10-07T13:30:00Z

@elasticmachine merge upstream

cchaos · 2020-10-07T15:32:03Z

src/core/public/chrome/ui/header/elastic_mark.tsx

+    <title id="elasticMark">Elastic</title>
+    <path
+      d="M9.74 16.882l.711-.076.046 1.455c-1.879.257-3.485.393-4.818.393-1.773 0-3.03-.515-3.773-1.545C1.164 16.08.8 14.473.8 12.306c0-4.333 1.727-6.5 5.167-6.5 1.666 0 2.909.47 3.727 1.394.818.924 1.227 2.394 1.227 4.379l-.106 1.409H2.664c0 1.364.242 2.379.742 3.03.5.652 1.349.985 2.576.985 1.242.03 2.485-.015 3.757-.121zm-.667-5.349c0-1.515-.243-2.59-.728-3.212-.484-.621-1.272-.94-2.363-.94s-1.924.334-2.47.986c-.545.651-.833 1.712-.848 3.166h6.409zM13.497 18.427V.7h1.848v17.727h-1.848zM27.027 9.806v6.076c0 .621 1.53.742 1.53.742l-.09 1.637c-1.303 0-2.38.106-3.03-.515a10.861 10.861 0 01-4.44.924c-1.136 0-2-.319-2.59-.97-.592-.636-.895-1.56-.895-2.773 0-1.197.303-2.09.91-2.651.605-.56 1.56-.925 2.863-1.046l3.879-.363v-1.06c0-.834-.182-1.44-.546-1.804-.363-.364-.863-.545-1.485-.545H18.27V5.82h4.742c1.394 0 2.41.318 3.046.97.651.636.97 1.651.97 3.015zm-7.606 5.03c0 1.516.621 2.273 1.879 2.273a9.89 9.89 0 003.303-.56l.56-.197v-4.076l-3.65.348c-.743.06-1.274.273-1.607.637-.333.363-.485.894-.485 1.575zM34.255 7.473c-1.788 0-2.697.62-2.697 1.879 0 .575.212.984.62 1.227.41.242 1.35.485 2.819.742 1.47.258 2.5.606 3.106 1.076.606.454.91 1.318.91 2.59 0 1.274-.41 2.198-1.228 2.789-.818.59-2 .894-3.576.894-1.015 0-4.424-.38-4.424-.38l.106-1.605c1.954.182 3.379.333 4.333.333.955 0 1.682-.151 2.182-.454.5-.303.758-.819.758-1.53 0-.713-.212-1.198-.637-1.455-.424-.258-1.363-.5-2.818-.728-1.455-.227-2.485-.56-3.09-1.015-.607-.439-.91-1.272-.91-2.47 0-1.196.424-2.09 1.273-2.666.848-.576 1.909-.864 3.166-.864 1 0 4.485.258 4.485.258v1.621c-1.833-.106-3.333-.242-4.379-.242zM47.952 7.685h-3.925v5.909c0 1.409.106 2.348.303 2.788.212.44.697.666 1.47.666l2.197-.151.121 1.53c-1.106.182-1.94.273-2.515.273-1.288 0-2.167-.318-2.667-.94-.5-.62-.742-1.818-.742-3.575v-6.5h-1.758V6.079h1.758V2.29h1.833v3.773h3.925v1.62zM50.527 3.276V1.139h1.849v2.152l-1.849-.015zm0 15.151V6.08h1.849v12.348h-1.849zM60.406 5.821c.546 0 1.47.106 2.773.303l.59.076-.075 1.5c-1.318-.152-2.288-.227-2.91-.227-1.393 0-2.348.333-2.848 1-.5.666-.757 1.909-.757 3.712 0 1.803.227 3.06.697 3.773.47.712 1.44 1.06 2.924 1.06l2.91-.227.075 1.53c-1.53.227-2.682.349-3.44.349-1.924 0-3.257-.5-3.984-1.485-.728-.985-1.106-2.652-1.106-5 0-2.349.393-4 1.181-4.94.803-.939 2.122-1.424 3.97-1.424z"
+      fill="#fff"


@snide I'd remove this fill. Then I think it's GTG

snide · 2020-10-07T15:35:51Z

@cchaos addressed

snide · 2020-10-07T15:36:24Z

@elasticmachine merge upstream

cchaos

Sweet, LGTM!

snide · 2020-10-15T18:08:20Z

@elasticmachine merge upstream

kibanamachine · 2020-10-15T19:45:50Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 50db7c6

Metrics [docs]

@kbn/optimizer bundle module count

id	before	after	diff
`core`	441	451	+10

distributable file count

id	before	after	diff
`default`	48195	48196	+1
`oss`	28633	28634	+1

page load bundle size

id	before	after	diff
`core`	655.1KB	661.6KB	+6.4KB

History

💚 Build #80495 succeeded b5e7995
💚 Build #80464 succeeded 5c4c09a
💔 Build #80370 failed 26b95c5
💔 Build #80366 failed 0339431
💔 Build #80254 failed 98477ed

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

snide · 2020-10-16T15:23:31Z

I'm gonna merge this for 7.11. @pgayvallet if you have a chance to improve the subscription check over the next minor please do. We have plenty of time to adjust it.

…80847) Puts a loader on the logo and replaces the bar that normally showed up.

* master: (51 commits) [Discover] Unskip flaky test (elastic#80670) Fix security solution template label (elastic#80754) [Ingest]: ignore 404, check if there are transforms in results. (elastic#80721) Moving loader to logo in header, add a slight 250ms pause (elastic#78879) [Security Solution][Cases] Fix bug with case connectors (elastic#80642) Update known-plugins.asciidoc (elastic#75388) [Lens] Add median operation (elastic#79453) Fix navigateToApp logic when navigating to the current app. (elastic#80809) [Visualizations] Fix bad color mapping with multiple split series (elastic#80801) [ILM] Add esErrorHandler for the new es js client (elastic#80302) Fix codeowners (elastic#80826) skip flaky suite (elastic#79463) [Timelion] Remove kui usage (elastic#80287) [Ingest Manager] add skipIfNoDockerRegistry to package_install_complete test (elastic#80779) [Alerting UI] Disable "Save" button for Alerts with broken Connectors (elastic#80579) Allow the default space to be accessed via `/s/default` (elastic#77109) Add script to identify plugin dependencies for TS project references migration (elastic#80463) [Search] Client side session service (elastic#76889) feat: 🎸 add separator for different context menu groups (elastic#80498) Lazy load reporting (elastic#80492) ...

Moving loading to logo, add a sligh 250ms pause

79a001c

snide requested review from a team as code owners September 29, 2020 22:07

snide requested a review from a team September 29, 2020 22:07

snide added release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0 labels Sep 29, 2020

snide changed the title ~~Moving leader to logo in header, add a slight 250ms pause~~ Moving loader to logo in header, add a slight 250ms pause Sep 29, 2020

ts expect error for testing

31ffea1

snide commented Sep 29, 2020

View reviewed changes

src/core/public/chrome/ui/header/header_logo.tsx Outdated Show resolved Hide resolved

cchaos and others added 3 commits September 29, 2020 18:55

Convert mark to a TS element

d87f175

- Removed inline fills for CSS implementation - Use the `euiHeaderLogo` class and remove custom styles

remove common props

744bbb3

i18n

da57992

pgayvallet reviewed Sep 30, 2020

View reviewed changes

mshustov reviewed Sep 30, 2020

View reviewed changes

src/core/public/chrome/ui/header/elastic_mark.tsx Outdated Show resolved Hide resolved

mshustov reviewed Sep 30, 2020

View reviewed changes

src/core/public/chrome/ui/loading_indicator.tsx Show resolved Hide resolved

snide added 2 commits September 30, 2020 19:08

smaller svg

75fd6d8

update tests

c25bf1f

ryankeairns approved these changes Oct 1, 2020

View reviewed changes

cchaos reviewed Oct 1, 2020

View reviewed changes

myasonik reviewed Oct 1, 2020

View reviewed changes

snide added 3 commits October 6, 2020 08:33

Merge remote-tracking branch 'upstream/master' into loader

98477ed

feedback

0339431

tests

26b95c5

Merge branch 'master' into loader

5c4c09a

cchaos suggested changes Oct 7, 2020

View reviewed changes

feedback

1e81d25

Merge branch 'master' into loader

b5e7995

cchaos approved these changes Oct 7, 2020

View reviewed changes

Merge branch 'master' into loader

50db7c6

snide merged commit 98c751a into elastic:master Oct 16, 2020

snide deleted the loader branch October 16, 2020 15:24

snide mentioned this pull request Oct 16, 2020

[7.x] Moving loader to logo in header, add a slight 250ms pause (#78879) #80847

Merged

snide added a commit that referenced this pull request Oct 16, 2020

Moving loader to logo in header, add a slight 250ms pause (#78879) (#…

5122530

…80847) Puts a loader on the logo and replaces the bar that normally showed up.

pgayvallet mentioned this pull request Nov 20, 2020

[GS] add search syntax support #83422

Merged

2 tasks

Desvelao mentioned this pull request Feb 20, 2023

[Backport 4.5-7.16] Fix cannot read null properties bug in settings section wazuh/wazuh-dashboard-plugins#5216

Merged

Conversation

snide commented Sep 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Demo

Checklist

For maintainers

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snide Sep 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgayvallet Oct 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryankeairns left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snide commented Oct 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snide commented Oct 7, 2020

Uh oh!

snide commented Oct 7, 2020

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

snide commented Oct 15, 2020

Uh oh!

kibanamachine commented Oct 15, 2020

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

distributable file count

page load bundle size

History

Uh oh!

snide commented Oct 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

snide commented Sep 29, 2020 •

edited

Loading

snide Sep 30, 2020 •

edited

Loading

pgayvallet Oct 1, 2020 •

edited

Loading