[Embeddables] Update `EmbeddablePanel` component to trigger edit panel action on error click #140205

dokmic · 2022-09-07T15:18:30Z

Summary

Resolves #138608.

Embeddable.Error.Click.mov

Checklist

Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2022-09-08T07:13:42Z

Pinging @elastic/kibana-app-services (Team:AppServicesUx)

vadimkibana

LGTM

MichaelMarcialis · 2022-09-12T16:20:45Z

@dokmic: Would you mind adding some screenshots or a short gif/video of the changes? Thanks!

dokmic · 2022-09-13T15:35:36Z

@MichaelMarcialis I've updated the summary to show a video. Would you mind taking another look?

MichaelMarcialis

Thanks for adding that video, @dokmic. I've added a couple of questions and comments for your review below. Let me know if you have any questions.

MichaelMarcialis · 2022-09-14T13:52:01Z

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

+    <EuiPanel
+      element="div"
+      className="embPanel__content embPanel__content--error"
+      color="transparent"
+      paddingSize="none"
+      data-test-subj="embeddableError"
+      panelRef={ref}
+      role={isEditable ? 'button' : undefined}
+      aria-label={isEditable ? ariaLabel : undefined}
+      onClick={handleErrorClick}
+    />


Rather than making a clickable EuiPanel, would it be better to use an EuiEmptyPrompt with an EuiButton in the actions prop (see examples in EUI docs)? I imagine that a dedicated button that notifies the user that they will be taken to edit the erroneous visualization would be a more user-friendly experience. Thoughts?

@MichaelMarcialis I've checked the component and the example. I don't think it is applicable in our case:

The panel here is just a dummy element we use as a placeholder for the UI -- embeddable widgets may render their custom error component. So that we cannot place a button here as it will not be shown in most of the cases.

This change introduces a generic click handler and doesn't touch any UI.

The UI present in the video comes from the Visualizations plugin, and it's made in a way to look similar to the error message from the Lens plugin.

If we would like to change the way it looks, we should create a separate issue that will request updates of the error appearance in three different places at the same time: Lens, Visualizations, and generic Embeddables.

Thanks for that explanation, @dokmic. Just to be sure I'm understanding, you're saying that EmbeddablePanelError is a placeholder that can be swapped out with a custom error component, correct? Is the swapping out with a custom error component required? Or is it optional, leaving the possibility for this "placeholder" error message to show instead?

If the swapping out of the EmbeddablePanelError component is optional (leaving the possibility for this "placeholder" error panel to be shown to users), I'm inclined to maintain my suggestion that we should instead utilize EuiEmptyPrompt here (with a conditional EuiButton that takes users to edit the visualization). Visually, I believe EuiEmptyPrompt can be styled to look the same as the error message shown in your example (for the sake of consistency with other error messages), just with the addition of the button for better usability.

Let me know if that makes sense or if I'm misunderstanding anything. CCing @flash1293, in case he has any thoughts.

Not holding this opinion strongly but I expected to use the context menu for this - click the cogwheel and select the edit action from there.

@MichaelMarcialis

EmbeddablePanelError is a placeholder that can be swapped out with a custom error component, correct? Is the swapping out with a custom error component required?

Yes, that's correct. It is optional but is overridden in majority of the places.

If the swapping out of the EmbeddablePanelError component is optional (leaving the possibility for this "placeholder" error panel to be shown to users), I'm inclined to maintain my suggestion that we should instead utilize EuiEmptyPrompt here (with a conditional EuiButton that takes users to edit the visualization).

I've checked the code once again, and the change seems impossible at the moment without a significant refactoring. The problem is that we have some internal service to open the panel for edit. It would be tricky to initialize that from Lens or Visualizations. The best solution is to get rid of the custom error components that look similar and use the fallback. But that should be done separately as it requires more work than the original issue and involves more parties.

Is that okay if I create a separate ticket to address that?

MichaelMarcialis · 2022-09-14T13:53:10Z

src/plugins/embeddable/public/lib/panel/_embeddable_panel.scss

+  .embPanel__content--error {
+    &:hover {
+      box-shadow: none;
+      transform: none;
+    }
+  }


If taking my suggestion to use an EuiEmptyPrompt with EuiButton, these styles will likely no longer be necessary.

MichaelMarcialis

I've checked the code once again, and the change seems impossible at the moment without a significant refactoring. The problem is that we have some internal service to open the panel for edit. It would be tricky to initialize that from Lens or Visualizations. The best solution is to get rid of the custom error components that look similar and use the fallback. But that should be done separately as it requires more work than the original issue and involves more parties.

Is that okay if I create a separate ticket to address that?

Thanks, @dokmic. If ya'll are willing to create that separate issue to help implement the above design suggestions, that sounds like a reasonable approach to me. Approving now so I don't hold you up further.

…ror click

kibana-ci · 2022-09-23T08:42:12Z

💚 Build Succeeded

Buildkite Build
Commit: 56789e4

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`embeddable`	83	84	+1

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`embeddable`	415	412	-3

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`embeddable`	69.2KB	70.0KB	+835.0B

Unknown metric groups

API count

id	before	after	diff
`embeddable`	515	512	-3

History

💚 Build #71833 succeeded 7fd7b1c
💚 Build #70137 succeeded 13acb6b

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

dokmic · 2022-09-23T16:16:49Z

I've created #141676 to address the UI updates as it was discussed above.

cc @MichaelMarcialis

… action on error click (elastic#140205)

dokmic added review Feature:Embedding Embedding content via iFrame release_note:skip Skip the PR/issue when compiling release notes Team:AppServicesUx v8.5.0 labels Sep 7, 2022

dokmic marked this pull request as ready for review September 8, 2022 07:13

dokmic requested review from a team as code owners September 8, 2022 07:13

vadimkibana approved these changes Sep 9, 2022

View reviewed changes

dokmic force-pushed the feature/138608 branch from 13acb6b to 7fd7b1c Compare September 13, 2022 15:31

MichaelMarcialis reviewed Sep 14, 2022

View reviewed changes

MichaelMarcialis approved these changes Sep 22, 2022

View reviewed changes

Update EmbeddablePanel component to trigger edit panel action on er…

56789e4

…ror click

dokmic force-pushed the feature/138608 branch from 7fd7b1c to 56789e4 Compare September 23, 2022 07:34

dokmic added v8.6.0 and removed v8.5.0 labels Sep 23, 2022

dokmic mentioned this pull request Sep 23, 2022

[Embeddable] Provide unified error UI #141676

Closed

dokmic merged commit 48b2ee4 into elastic:main Sep 23, 2022

kibanamachine added the backport:skip This commit does not require backporting label Sep 23, 2022

dokmic deleted the feature/138608 branch September 23, 2022 16:17

jennypavlova pushed a commit to jennypavlova/kibana that referenced this pull request Sep 27, 2022

[Embeddable] Update EmbeddablePanel component to trigger edit panel…

4785fab

… action on error click (elastic#140205)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Embeddables] Update `EmbeddablePanel` component to trigger edit panel action on error click #140205

[Embeddables] Update `EmbeddablePanel` component to trigger edit panel action on error click #140205

dokmic commented Sep 7, 2022 •

edited

Loading

elasticmachine commented Sep 8, 2022

vadimkibana left a comment

MichaelMarcialis commented Sep 12, 2022

dokmic commented Sep 13, 2022

MichaelMarcialis left a comment

MichaelMarcialis Sep 14, 2022

dokmic Sep 15, 2022

MichaelMarcialis Sep 20, 2022

flash1293 Sep 20, 2022

dokmic Sep 21, 2022 •

edited

Loading

MichaelMarcialis Sep 14, 2022

MichaelMarcialis left a comment

kibana-ci commented Sep 23, 2022

API count

dokmic commented Sep 23, 2022 •

edited

Loading

[Embeddables] Update EmbeddablePanel component to trigger edit panel action on error click #140205

[Embeddables] Update EmbeddablePanel component to trigger edit panel action on error click #140205

Conversation

dokmic commented Sep 7, 2022 • edited Loading

Summary

Checklist

elasticmachine commented Sep 8, 2022

vadimkibana left a comment

Choose a reason for hiding this comment

MichaelMarcialis commented Sep 12, 2022

dokmic commented Sep 13, 2022

MichaelMarcialis left a comment

Choose a reason for hiding this comment

MichaelMarcialis Sep 14, 2022

Choose a reason for hiding this comment

dokmic Sep 15, 2022

Choose a reason for hiding this comment

MichaelMarcialis Sep 20, 2022

Choose a reason for hiding this comment

flash1293 Sep 20, 2022

Choose a reason for hiding this comment

dokmic Sep 21, 2022 • edited Loading

Choose a reason for hiding this comment

MichaelMarcialis Sep 14, 2022

Choose a reason for hiding this comment

MichaelMarcialis left a comment

Choose a reason for hiding this comment

kibana-ci commented Sep 23, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Page load bundle

API count

History

dokmic commented Sep 23, 2022 • edited Loading

[Embeddables] Update `EmbeddablePanel` component to trigger edit panel action on error click #140205

[Embeddables] Update `EmbeddablePanel` component to trigger edit panel action on error click #140205

dokmic commented Sep 7, 2022 •

edited

Loading

dokmic Sep 21, 2022 •

edited

Loading

dokmic commented Sep 23, 2022 •

edited

Loading