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.

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

[Uptime] Repair Alert Flyout #73528

Merged
merged 10 commits into from
Jul 30, 2020

Conversation

justinkambic
Copy link
Contributor

Summary

Fixes #73527.

This PR fixes the alert flyout in the Uptime solution UI.

An animation of the current state of the flyout is available in the linked issue. The result of this change is depicted below:

Jul-28-2020 13-59-01

Testing this PR

  1. Open the Uptime Solution
  2. Open the Monitor Status alert flyout
  3. Ensure the flyout loads

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@justinkambic justinkambic added bug Fixes for quality problems that affect the customer experience v8.0.0 Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.10.0 v7.9.0 labels Jul 28, 2020
@justinkambic justinkambic requested a review from shahzad31 July 28, 2020 18:08
@justinkambic justinkambic requested a review from a team as a code owner July 28, 2020 18:08
@justinkambic justinkambic self-assigned this Jul 28, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@justinkambic justinkambic added the release_note:skip Skip the PR/issue when compiling release notes label Jul 28, 2020
@justinkambic justinkambic force-pushed the uptime_repair-alert-flyout branch from 6bbea81 to f5e2705 Compare July 28, 2020 20:16
@justinkambic
Copy link
Contributor Author

@shahzad31 I realized my previous patch won't work because it doesn't satisfy the typing constraint. The validation function isn't allowed to return a Promise. Let me know if the change in f5e2705 works for you. I've removed the validation function's dependency on triggers_actions_ui, so it isn't depending on any external plugin code, and the inferred type it returns works for the API typing.

@shahzad31
Copy link
Contributor

@shahzad31 I realized my previous patch won't work because it doesn't satisfy the typing constraint. The validation function isn't allowed to return a Promise. Let me know if the change in f5e2705 works for you. I've removed the validation function's dependency on triggers_actions_ui, so it isn't depending on any external plugin code, and the inferred type it returns works for the API typing.

@justinkambic why not update types to handle this use case? change is fine, it's just it will increase uptime page load bundle size by adding fp-ts etc into it. Earlier it was lazy loaded only when validate was called.

@justinkambic
Copy link
Contributor Author

justinkambic commented Jul 29, 2020

@mikecote do you think it would be an acceptable change to convert the AlertType's validate function type to ValidationResult | Promise<ValidationResult>? It would obviously require some revision within triggers_actions_ui as well.

EDIT: disregard, Shahzad came up with a better solution that requires no changes.

@@ -10,7 +10,7 @@ import {
AtomicStatusCheckParamsType,
MonitorAvailabilityType,
StatusCheckParamsType,
} from '../../../../common/runtime_types/alerts';
} from '../../../../common/runtime_types';
Copy link
Contributor

Choose a reason for hiding this comment

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

you will also need to revert this, actually when you import it from index.ts file, it ends up bundling everything from that index file into the imported bundle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing that out. I had consider reverting it and didn't think it would impact. I was wrong! Fixed this in b1b8b78.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

page load bundle size

id value diff baseline
uptime 23.7KB +2.0B 23.7KB

History

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

Copy link
Contributor

@shahzad31 shahzad31 left a comment

Choose a reason for hiding this comment

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

LGTM !!

@justinkambic justinkambic merged commit 6d858ac into elastic:master Jul 30, 2020
shahzad31 added a commit to shahzad31/kibana that referenced this pull request Jul 30, 2020
* Add await to promise.

* Remove lazy loading and reference to lazy resource from validation.

* Revert previous changes and add simpler fix.

* Remove unnecessary formatting changes.

* Further cleanup.

* Add more descriptive import path.

* fix type

* fix type

Co-authored-by: Shahzad <[email protected]>
@justinkambic
Copy link
Contributor Author

This PR didn't get backported directly. Because this PR is a fix for a defect previously-injected, we cherry-picked this commit to an existing backport PR #73733.

@justinkambic justinkambic deleted the uptime_repair-alert-flyout branch July 31, 2020 15:01
@shahzad31 shahzad31 added the backport:skip This commit does not require backporting label Jul 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Uptime] Alert flyout is broken
4 participants