Skip to content

Conversation

@YulNaumenko
Copy link
Contributor

@YulNaumenko YulNaumenko commented Nov 12, 2020

Current PR include the next changes:

  1. Replaced AppContextProvider with KibanaContextProvider.
  2. Released number of props which paths through the alerting components by replacing this with useKibana().services
  3. Removed ActionsConnectorsContextProvider
  4. Replaces mocks for context dependancies with kibana_react.mock. Unit tests code reduced and simplified.
  5. Extended TriggersAndActionsUIPublicPluginStart with exposed connector flyouts: AddConnectorFlyout and EditConnectorFlyout. This allows to reduce the number of pathed dependancies and remove require of the code directly from our plugins.

Resolve #65146

@YulNaumenko YulNaumenko added Feature:Alerting v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// v7.11.0 labels Nov 12, 2020
@YulNaumenko YulNaumenko self-assigned this Nov 12, 2020
@YulNaumenko YulNaumenko force-pushed the alerting-ui-introduce-kibana-context branch from 8dab255 to ee73051 Compare November 18, 2020 17:29
@YulNaumenko YulNaumenko marked this pull request as ready for review November 18, 2020 20:17
@YulNaumenko YulNaumenko requested review from a team as code owners November 18, 2020 20:17
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

@botelastic botelastic bot added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Nov 18, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@YulNaumenko YulNaumenko requested a review from gmmorris November 19, 2020 19:05
@shahzad31
Copy link
Contributor

@YulNaumenko now it seems to work fine, but when i click create connector is uptime settings, it seems to open flyout twice, there is some flaky behaviour , i will try to post a gif later on.

image

Copy link
Contributor

@gmmorris gmmorris left a comment

Choose a reason for hiding this comment

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

I've checked the lazy loading chuck size and the UX throughout Alerts Management and flyouts in solutions and looks like everything is still working as expected. 👍

@gmmorris
Copy link
Contributor

gmmorris commented Nov 20, 2020

@YulNaumenko now it seems to work fine, but when i click create connector is uptime settings, it seems to open flyout twice, there is some flaky behaviour , i will try to post a gif later on.

image

I tried to reproduce and couldn't - are you sure you've pulled the latest changes?

NVM, I was looking at the Alerts flyout, not the Connectors.
It does seem to double render... possibly there's something causing unnecessary renders caused by a missing useMemo or useCallback

Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

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

Hi! Thank you so much for doing this. I tested locally and it seems there are some flaky behavior and an error.

  1. The edit flyout seems to open and close multiple times when you update credentials:

flak_01

  1. When you enter the configuration page of cases it seems that one of the flyouts opens and closes instantly:

flak_02

  1. When you press the test tab you get the following error:

bug_01

@YulNaumenko YulNaumenko requested a review from cnasikas November 20, 2020 23:10
Copy link
Contributor

@phillipb phillipb left a comment

Choose a reason for hiding this comment

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

Looks good for logs and metrics.

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.

Uptime changes LGTM !!

Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

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

Security Solution changes LGTM! All issues have been resolved! Thank you!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
triggersActionsUi 282 283 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 7.9MB 7.9MB -587.0B
triggersActionsUi 1.5MB 1.5MB +21.1KB
uptime 984.1KB 983.7KB -499.0B
total +20.0KB

Distributable file count

id before after diff
default 43058 43052 -6

Page load bundle

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

id before after diff
triggersActionsUi 154.7KB 161.0KB +6.2KB
Unknown metric groups

async chunk count

id before after diff
triggersActionsUi 33 31 -2

History

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

@YulNaumenko YulNaumenko merged commit b11f783 into elastic:master Nov 24, 2020
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 26, 2020
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

1 similar comment
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.11.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Alerting UI] Replace some of ui plugin dependencies with KibanaContextProvider

9 participants