Improve Embeddable loading and error handling UI #68623

lizozom · 2020-06-09T08:44:41Z

Summary

This PR is an enhancement based on @mdefazio mocks for Make it Slow.

In visualize_embeddable and search_embeddable UI, we now:

Gray out previous visualization data while loading new data (Thanks @markov00 for the idea)
Gray out the panel title while loading
On error, keep data grayed out and show an Error or a Aborted label to each visualization. Users can simply use the back button if they wish to return to the old search config.

Note that other app behavior did not change and still shows an error toast notification in discover and visualize.

These changes can be easily applied to other embeddables (Lens @elastic/kibana-app and Maps @elastic/kibana-gis for example) to create a uniform user experience. This can be achieved by using the embeddable's output and setting it to:

Loading: this.updateOutput({ loading: true, error: undefined });
Complete: this.updateOutput({ loading: false, error: undefined });
Error: this.updateOutput({ loading: false, error });

Added benefits

Any individual embeddable error will be now handled by the embeddable, instead of being output into the toasts area. This change partially resolves a known issue where on search cancellation \ errors we trigger multiple toasts.
This PR also resolves an issue where a user aborts a search using search_interceptor, but the expression's AbortController is not cancelled properly (it didn't have any implications ATM, but would have them with Make it Slow).

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, including a check against IE11

For maintainers

This was checked for breaking API changes and was labeled appropriately

Gray out loading \ errored results

elasticmachine · 2020-06-09T08:44:44Z

Pinging @elastic/kibana-app-arch (Team:AppArch)

…ize-embeddable-error-handling

src/plugins/data/public/search/search_interceptor.ts

src/plugins/visualizations/public/embeddable/_embeddables.scss

src/plugins/visualizations/public/embeddable/visualize_embeddable.tsx

Dosant · 2020-06-09T09:39:15Z

src/plugins/visualizations/public/embeddable/visualize_embeddable.tsx

-    this.handler = new expressions.ExpressionLoader(this.domNode);
+    this.handler = new expressions.ExpressionLoader(this.domNode, '', {
+      onRenderError: (element: HTMLElement, error: RenderError) => {
+        this.onContainerError(error);


I haven't test yet, but I wonder:

considering very recent fixes around race conditions in this file, could this callback become another source for possible race condition?

Like for example: there is already a new request finished and new vis rendered, but somehow error from previous request popped up and replaced actual visualization with older error?

There are some checks happening in updateHander, but it relies on reference to older abort controller.

This callback is called exactly the same way it was previously - just with different UI logic.

See src/plugins/expressions/public/render.ts line 68.

We used to use defaultRenderErrorHandler before, and it would output the errors as toasts. Since we now pass a custom error handler - it gets called instead when an error occurs.

There are no other changes in flow. I just extracted the state handling into meaningful callbacks to make it more readable.

I think there is a slight difference:

Previously we just showed a popup on the right and error didn't affect the visualisation embeddable.

Now if older error pops up we will show "error" label near the visualisation in actual state.

I still haven't reproduced what I am worrying about here :(
Just being extra cautions because of recent experience with cancelations :(
Don't have a particular action item here now

src/plugins/visualizations/public/embeddable/_embeddables.scss

Dosant · 2020-06-09T10:19:23Z

Is this graying out change OK from a11y perspective? Maybe someone have to look at it if haven't yet?
I don’t have experience with compliance in this field, etc, so I can't tell..
I guess, if we treat it as an enchantment, it is fine.
Just rising some concerns I am not 100% sure about:

Not everyone would recognise the difference loading and not loading state?
1.1 because of that I guess we still have to add loading indicator on top of embeddable panel?
Could it be harmful for someone that we change color palette on a fly that quickly? This especially could cause major color shifts on a dashboard, where we have a lot of visualisations

On a dashboard only visualizations will use this gray behaviour. but not lens, maps, etc. Is this an issue?

Dosant · 2020-06-09T10:21:47Z

src/plugins/visualizations/public/embeddable/visualize_embeddable.tsx

+  };
+
+  onContainerError = (error: RenderError) => {
+    if (this.abortController) {


This is related to: https://github.com/elastic/kibana/pull/68623/files#r437278595

I guess here could be a race condition, that when error pops up, this.abortController is actually an abortController for new on-going request which haven't finished yet?

The graying out will actually prevent the user from applying a filter while another search is running, reducing the chances of this happening.

lizozom · 2020-06-09T11:25:00Z

@elasticmachine merge upstream

src/plugins/visualizations/public/embeddable/_embeddables.scss

src/plugins/visualizations/public/embeddable/embeddable_error_label.tsx

src/plugins/visualizations/public/embeddable/_embeddables.scss

cchaos · 2020-06-09T14:50:15Z

The general rule of thumb with a11y is that if something is disabled (can’t interact with it) or in this case doesn’t actually represent any (current) data, you don’t really need to adhere to contrast levels. Though we tend to try to ensure at least a 2.0 ratio. As long as there is an indicator as to why there’s no content or grayed content, such as your label, it should be ok for a11y with regards to color.

What you might need to look into is ensuring that for screen-readers, it is announced appropriately as either loading, incomplete, or has error.

…ize-embeddable-error-handling

src/plugins/embeddable/public/lib/panel/embeddable_panel.tsx

cchaos

Code-wise this LGTM, but I'm going to defer the design review to @mdefazio.

mdefazio · 2020-06-10T15:15:00Z

Design looks good. I think one update we need to look into is also dimming the title as well. For non-visual panels, there won't be any difference between loading/loaded. So the idea is that this would help here. I don't know that there is enough difference with the Text Dark shade, so I think I'd try $euiColorMediumShade.

Integrate with search embeddable

mdefazio

Design LGTM!

…ize-embeddable-error-handling

lizozom · 2020-06-11T18:53:14Z

@elasticmachine merge upstream

ThomThomson

LGTM

What's especially nice is the instant feedback given on user action. The greyed out chart/title says "I'm working, give me a second" which makes everything feel snappier.

One thing to consider though, are situations in which the embeddable panel might be loading, but not because of direct user action. Such as an auto-refresh. In some use-cases, it's just expected that the data is up to date, and a distinct loading state just adds noise.

Not urgent, and shouldn't get in the way of merging this, but I wonder if it would be worth adding some kind of tag that determines when to show the loading state.

kibanamachine · 2020-06-11T20:38:52Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 282c44c

History

💚 Build #53277 succeeded 65ff155
💔 Build #52965 failed e5083e5
💔 Build #52766 failed 97139b4
💔 Build #52680 failed eb48c17
💔 Build #52663 failed 1310545

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

* Add error label on visualize embeddable error Gray out loading \ errored results * Translate label * code review * Rename ExpressionRenderError, better scss * oopsy * Generalize solution to be used by other embeddables * Cleanup * css fix * Move wrapper div into label component * Lighten panel title while loading Integrate with search embeddable * translations * Error notification in visualize app * Update render count on error Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* Add error label on visualize embeddable error Gray out loading \ errored results * Translate label * code review * Rename ExpressionRenderError, better scss * oopsy * Generalize solution to be used by other embeddables * Cleanup * css fix * Move wrapper div into label component * Lighten panel title while loading Integrate with search embeddable * translations * Error notification in visualize app * Update render count on error Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

piyushabad88 · 2021-02-03T14:03:19Z

Hi Team,

In which stable version this issue fixed. we are using 7.9.3 and facing same issue. Looking for prompt response .

markov00 · 2021-02-16T09:30:32Z

Hi @piyushabad88, looks like it's released on 7.9 at least. Is your issue related to #71369 or #82110?
If not please can you open a GH issue linking to this PR? thanks
cc @lizozom

Hi Team,

In which stable version this issue fixed. we are using 7.9.3 and facing same issue. Looking for prompt response .

piyushabad88 · 2021-02-17T05:16:03Z

Hi, Thanks for consideration. This issue related to #71369 <#71369> Thanks, PIyush

…

On Tue, Feb 16, 2021 at 3:00 PM Marco Vettorello ***@***.***> wrote: Hi @piyushabad88 <https://github.com/piyushabad88>, looks like it's released on 7.9 at least. Is your issue related to #71369 <#71369> or #82110 <#82110>? If not please can you open a GH issue linking to this PR? thanks cc @lizozom <https://github.com/lizozom> Hi Team, In which stable version this issue fixed. we are using 7.9.3 and facing same issue. Looking for prompt response . — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#68623 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGNFR2FUO3GYRC6DGQTJ6N3S7I3MTANCNFSM4NZF4PZQ> .

Add error label on visualize embeddable error

72fc079

Gray out loading \ errored results

lizozom added Feature:Search Querying infrastructure in Kibana enhancement New value added to drive a business result v8.0.0 Team:AppArch release_note:skip Skip the PR/issue when compiling release notes v7.9.0 labels Jun 9, 2020

lizozom requested review from a team as code owners June 9, 2020 08:44

lizozom self-assigned this Jun 9, 2020

botelastic bot added the Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) label Jun 9, 2020

Liza K added 2 commits June 9, 2020 11:50

Translate label

5ae4ad3

Merge branch 'master' of github.com:elastic/kibana into search/visual…

35141a8

…ize-embeddable-error-handling

lizozom requested a review from mdefazio June 9, 2020 08:52

lizozom removed the Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) label Jun 9, 2020