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.

Sign up for GitHub

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

Jump to bottom

[Discover] Deangularization context error message refactoring #70090

Merged
merged 7 commits into from
Jul 1, 2020
Merged

[Discover] Deangularization context error message refactoring #70090

merged 7 commits into from
Jul 1, 2020

Conversation

dej611
Copy link
Contributor

@dej611 dej611 commented Jun 26, 2020 •
edited
Loading

Summary

This PR contains a deangularization of the Context Error Message component, re-implemented with the new EUI package.

  • ✨ Created a new ContextErrorMessage component in React
  • 💄 Used the new EUI Callout component for the error message
  • ✅ Added tests for different scenarios

No error scenario:
Screenshot 2020-06-26 at 17 30 17

Title only error scenario:
Screenshot 2020-06-26 at 17 28 54

Unknown error scenario:
Screenshot 2020-06-26 at 17 29 52

Updated
In the original version there was a tiebreaker special error handling which has been removed in this version, due to impossibility to reproduce the error. Therefore the conditional handler branch has been removed in this implementation.

Checklist

Delete any items that are not applicable to this PR.

@dej611 dej611 added Feature:Discover Discover Application Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.9.0 labels Jun 26, 2020
@dej611 dej611 requested a review from kertal June 26, 2020 16:17
@dej611 dej611 self-assigned this Jun 26, 2020
@dej611 dej611 changed the title [Deangularization] Discover context error message refactoring [Discover] Deangularization context error message refactoring Jun 26, 2020
@dej611 dej611 force-pushed the deangularization/discover-context-error-message branch from d057fbb to 41fb803 Compare June 29, 2020 08:33
@dej611
Copy link
Contributor Author

dej611 commented Jun 29, 2020

@elasticmachine merge upstream

kertal
kertal reviewed Jun 29, 2020
View reviewed changes
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Code LGTM 👍 , let's check because of the redundant message before merging it

...ugins/discover/public/application/components/context_error_message/context_error_message.tsx Outdated
Comment on lines 58 to 67
{reason === FAILURE_REASONS.INVALID_TIEBREAKER && (
<FormattedMessage
id="discover.context.noSearchableTiebreakerFieldDescription"
defaultMessage="No searchable tiebreaker field could be found in the index pattern {indexPatternId}. Please change the advanced setting {tieBreakerFields} to include a valid field for this index pattern."
values={{
indexPatternId: queryParameters.indexPatternId,
tieBreakerFields: <code>context:tieBreakerFields</code>,
}}
/>
)}
Copy link
Member

Choose a reason for hiding this comment

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

I think this error message is redundant, legacy, might have been useful in recent time. I tried to set the tieBreakerFields to an invalid value, but I that case _doc is used as tiebreaker. And I think this is fine this way. So unless you find a way to break this to have this message displayed, I'd suggest to remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried some ideas but didn't work. I'll remove it.

...ugins/discover/public/application/components/context_error_message/context_error_message.tsx Outdated
*/
reason?: string;
/**
* parameters used for invalid tieBreakerFields realted errors
Copy link
Member

Choose a reason for hiding this comment

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

Nit: "realted" sounds bit like Spanish ⚽, I think it's "related"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given the removal of the error above I guess I can remove also this prop?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, pls clean up dead code, thx

@dej611 dej611 requested a review from kertal June 30, 2020 10:16
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
discover 83 +3 80

History

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

kertal
kertal approved these changes Jun 30, 2020
View reviewed changes
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Code LGTM 👨‍🎨, thx for taking care of another brick in the Discover wall. Useful PR description + images (you can skip the part of the tiebreaker failure or convert it into info that it was removed), tested locally in Chrome, Firefox, Safari, Mac OS 10.14.6, works 👍

@dej611 dej611 marked this pull request as ready for review July 1, 2020 08:41
@dej611 dej611 requested a review from a team July 1, 2020 08:41
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@dej611 dej611 merged commit 22a41c5 into elastic:master Jul 1, 2020
@dej611 dej611 deleted the deangularization/discover-context-error-message branch July 1, 2020 08:54
@dej611 dej611 mentioned this pull request Jul 1, 2020
Merged
dej611 added a commit to dej611/kibana that referenced this pull request Jul 1, 2020
@dej611 @elasticmachine
[Discover] Deangularization context error message refactoring (elasti…
1be87e8 
…c#70090)

Co-authored-by: Elastic Machine <[email protected]>
Bamieh pushed a commit to Bamieh/kibana that referenced this pull request Jul 1, 2020
@dej611 @elasticmachine @Bamieh
[Discover] Deangularization context error message refactoring (elasti…
b4f3671 
…c#70090)

Co-authored-by: Elastic Machine <[email protected]>
dej611 added a commit that referenced this pull request Jul 1, 2020
@dej611 @elasticmachine
[7.x] [Discover] Deangularization context error message refactoring (#…
b263b74 
…70090) (#70406)

Co-authored-by: Elastic Machine <[email protected]>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 1, 2020
@gmmorris
Merge remote-tracking branch 'upstream/master' into alerting/consumer…
3c4a7a9 
…-based-rbac

* upstream/master: (38 commits)
  Move logger configuration integration test to jest (elastic#70378)
  Changes observability plugin codeowner (elastic#70439)
  update (elastic#70424)
  [Logs UI] Avoid CCS-incompatible index name resolution (elastic#70179)
  Enable "Explore underlying data" actions for Lens visualizations (elastic#70047)
  Initial work on uptime homepage API (elastic#70135)
  expressions indexPattern function (elastic#70315)
  [Discover] Deangularization context error message refactoring (elastic#70090)
  [Lens] Add "no data" popover (elastic#69147)
  [Lens] Move chart switcher over (elastic#70182)
  chore: add missing mjs extension (elastic#70326)
  [Lens] Multiple y axes (elastic#69911)
  skip flaky suite (elastic#70386)
  fix bug to add timeline to case (elastic#70343)
  [QA][Code Coverage] Drop catchError and use try / catch instead, (elastic#69198)
  [QA] [Code Coverage] Integrate with Team Assignment Pipeline and Add Research and Development Indexes and Cluster (elastic#69348)
  [Metrics UI] Add context.reason and alertOnNoData to Inventory alerts (elastic#70260)
  Resolver refactoring (elastic#70312)
  [Ingest Manager] Fix agent ack after input format change (elastic#70335)
  [eslint][ts] Enable prefer-ts-expect-error (elastic#70022)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kertal kertal kertal approved these changes

Assignees

@dej611 dej611

Labels
Feature:Discover Discover Application release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.9.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dej611 @kibanamachine @elasticmachine @kertal