-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Lens] Improves error messages when in Dashboard #90668
Conversation
@elasticmachine merge upstream |
Pinging @elastic/kibana-app (Team:KibanaApp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM! Code only review, mostly focused on Lens embeddable and expression wrapper.
@@ -15,3 +15,11 @@ | |||
position: static; // Let the progress indicator position itself against the outer parent | |||
} | |||
} | |||
|
|||
.lnsExpressionRendererEmbeddedError { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following our BEM naming convention, perhaps this should be .lnsExpressionRenderer__embeddedError
, similar to the class above (.lnsExpressionRenderer__component
).
Presuming that to be the case, should it also be nested in the group above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment the component using this class is not nested within a lnsExpressionRenderer
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the component using this class is not nested within lnsExpressionRenderer
, would it be better to name the class lnsEmbeddedError
in order to avoid confusion such as the above? Just helps further separate the two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
datasourceLayers, | ||
}), | ||
errors: validationResult, | ||
}; | ||
} | ||
|
||
export const validateDatasourceAndVisualization = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just realizing this function is only operating on a single datasource - I guess this is something we have to revisit once we introduce additional datasource that can be active at the same time (like SQL). Nothing for this PR though.
datasourceStates, | ||
datasourceLayers, | ||
}); | ||
{ datasourceLayers } as FramePublicAPI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a little scary - can be tighten the types here a bit? I went through our current consumers and there's one place using something besides datasourceLayers
- the xy visualization will check out activeData
.
Can we change the type of the second parameter of getConfiguration
to Pick<FramePublicAPI, 'datasourceLayers' | 'activeData'>
? This should fix the type cast and we want to move away from the kitchen sink api anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's also availablePalettes
which is required somewhere. I couldn't find a way to drop it for the embeddable so for now I'm loading them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Managed to isolate it and remove availablePalettes
as dependency as it was unused
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments/questions regarding these error messages. Nothing show-stopping, but just thought I'd bring them up. Let me know what you think.
<EuiFlexItem> | ||
<FormattedMessage | ||
id="xpack.lens.embeddable.moreErrors" | ||
defaultMessage="(Edit in Lens editor to see more errors)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than having in parenthesis, can we treat this like a regular sentence?
Also, is it possible to make Edit in Lens editor
a link that opens this visualization in Lens in a new tab? Offering the user a quick action to correct the problem would be nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than having in parenthesis, can we treat this like a regular sentence?
Also, is it possible to make
Edit in Lens editor
a link that opens this visualization in Lens in a new tab? Offering the user a quick action to correct the problem would be nice.
Tried to implement the Edit in Lens
button with the same logic of the Edit action (dropdown context menu) but it's not trivial. Perhaps it could be done as a follow up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! Let me know if you'd like me to open an issue for it.
<div className="lnsExpressionRenderer__embeddedError"> | ||
<EuiFlexGroup | ||
direction="column" | ||
justifyContent="center" | ||
alignItems="center" | ||
style={{ maxWidth: '100%' }} | ||
data-test-subj="embeddable-lens-failure" | ||
> | ||
<EuiFlexItem grow={false}> | ||
<EuiIcon type="alert" size="xl" color="danger" /> | ||
</EuiFlexItem> | ||
{customError || defaultMessage} | ||
{moreErrors} | ||
</EuiFlexGroup> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is using an EuiFlexGroup
as shown here the standard for displaying Dashboard error messages? I'd think it would be cleaner/easier to just use an EuiEmptyPrompt
instead, since it already bakes in icon, text and action buttons. You could then also put your messages in a more semantic p
elements, rather than the current EuiFlexItem
wrappers. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice idea. I used the same structure as in the Lens workspace but this one looks nicer.
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and works as expected, LGTM. Thanks for the functional test, I think this is a thing that can break easily without anyone noticing.
This reverts commit 34ec35d.
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and LGTM, but probably needs one more pass
title, | ||
description, | ||
const datasourceId = getActiveDatasourceIdFromDoc(doc); | ||
if (datasourceId == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strict equality works here, I'm surprised the linter allows ==
if (datasourceId == null) { | |
if (datasourceId === null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any particular meaning for null
and undefined
here, or do they have the same meaning?
eslint has a special option for null
or undefined
case in the rule eqeqeq
, which is usually enabled.
As in steps 2 and 3 of the ECMA262 Abstract Equality spec the ==
operator treat them the same exact way. So unless specific distinct meaning of the two there's no difference.
await embeddable.initializeSavedVis({} as LensEmbeddableInput); | ||
embeddable.render(mountpoint); | ||
|
||
expect(expressionRenderer).toHaveBeenCalledTimes(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This expect
isn't really asserting what the title of the test says it does, can you align the two?
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
I checked this PR last week and I forgot to confirm my comments 🤦 But all has been addressed in the meantime so approving! |
Co-authored-by: Kibana Machine <[email protected]>
…ndition-for-hiding-recommded-allocation * 'master' of github.com:elastic/kibana: [Discover] Fix toggling multi fields from doc view table (elastic#91121) [ML] Data Frame Analytics: ROC Curve Chart (elastic#89991) skip flaky suite (elastic#86948) skip flaky suite (elastic#91191) Fix date histogram time zone for rollup index (elastic#90632) [Search Source] Fix retrieval of unmapped fields; Add field filters (elastic#89837) [Logs UI] Use useMlHref hook for ML links (elastic#90935) Fix values of `products.min_price` field in Kibana sample ecommerce data set (elastic#90428) [APM] Darker shade for Error group details labels (elastic#91349) [Lens] Adjust new copy for 7.12 (elastic#90413) [ML] Unskip test. Fix modelMemoryLimit value. (elastic#91280) [Lens] Fix empty display name issue in XY chart (elastic#91132) [Lens] Improves error messages when in Dashboard (elastic#90668) [Lens] Keyboard-selected items follow user traversal of drop zones (elastic#90546) [Lens] Improves ranking feature in Top values (elastic#90749) [ILM] Rollover min age tooltip and copy fixes (elastic#91110) # Conflicts: # x-pack/plugins/index_lifecycle_management/__jest__/client_integration/edit_policy/edit_policy.test.ts
Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
Summary
Fixes #86278, #90594
This PR addresses some issues when embedding a Lens visualisation into Dashboard panels.
In particular visualisation would either not show an explaining error message when failing ( #86268 ) or rendering despite not being "valid" for Lens rules ( #90594 ).
The validation logic used for Lens editor has been now integrated into the embeddable to propagate the same experience.
Checklist