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

Port of appStatus from 4.x branch #8148

Merged
merged 9 commits into from
Sep 7, 2016
Merged

Conversation

BigFunger
Copy link
Contributor

@BigFunger BigFunger commented Sep 1, 2016

Closes #8068

Adds the appStatus introduced in #7984 to discover, dashboard, and visualize apps
Applies the cloneDeep fix from #8066 to state_manager
Applies changes to state_monitor_factory from #8082

Changes the root node type of dashboard from:
<div dashboard-app ...> ->'<dashboard-app ...>

Changes the root node type of discover from:
<div ng-controller="discover" ...> ->'<discover-app ...>

Changes the root node type of visualization editor from:
<div ng-controller="VisEditor" ...> ->'<visualize-app ...>

@epixa
Copy link
Contributor

epixa commented Sep 6, 2016

This needs to be rebased or have master merged

@w33ble
Copy link
Contributor

w33ble commented Sep 6, 2016

After merging master, and the CI runs successfully, this LGTM. Spoke too soon. There's another PR that needs to make its way into this one: #8082

@BigFunger BigFunger removed their assignment Sep 6, 2016
}

function getStatus() {
const currentState = removeIgnoredProps(cloneDeep(state.toJSON()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add this above this line: // state.toJSON returns a reference, clone so we can mutate it safely

@w33ble
Copy link
Contributor

w33ble commented Sep 6, 2016

2 very tiny changes, then this LGTM!

@w33ble
Copy link
Contributor

w33ble commented Sep 6, 2016

@cjcenizal you mind taking a quick look at this too, since you reviewed the original one? This is likely going to be the code you are going to be using with the app state changes I think.

app
.directive('discoverApp', function () {
return {
controllerAs: 'discoverApp',
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor suggestion: if we add restrict: 'E', then we can make the markup a bit cleaner (below).

@cjcenizal
Copy link
Contributor

Just a couple small suggestions, and then this LGTM too!

@BigFunger
Copy link
Contributor Author

@w33ble, @cjcenizal I applied the changes you both suggested.

app.controller('discover', function ($scope, config, courier, $route, $window, Notifier,
app.directive('discoverApp', function () {
return {
controllerAs: 'discoverApp',
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't forget to add restrict: 'E' here

.controller('VisEditor', function ($scope, $route, timefilter, AppState, $location, kbnUrl, $timeout, courier, Private, Promise) {
.directive('visualizeApp', function () {
return {
controllerAs: 'visualizeApp',
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs restrict: 'E' here too

@cjcenizal
Copy link
Contributor

LGTM!

@BigFunger BigFunger merged commit 0b472a3 into elastic:master Sep 7, 2016
airow pushed a commit to airow/kibana that referenced this pull request Feb 16, 2017
Port of appStatus from 4.x branch

Former-commit-id: 0b472a3
tiansivive added a commit that referenced this pull request Jan 10, 2024
…tity flyouts (#174438)

This PR adds the Asset Criticality selector to both the new User and
Host expandable flyouts

User flyout:
<img width="770" alt="Screenshot 2024-01-08 at 12 17 57"
src="https://github.com/elastic/kibana/assets/2423976/6c07de00-4863-47ed-83f5-67aec6354585">

Host flyout:
<img width="749" alt="Screenshot 2024-01-08 at 12 18 09"
src="https://github.com/elastic/kibana/assets/2423976/815ed517-954b-4ab5-9ccf-5f4ca28ec5c2">


Part of [#8148](elastic/security-team#8148)
and the parent epic
[#4208](elastic/security-team#4208)

---------

Co-authored-by: Kibana Machine <[email protected]>
delanni pushed a commit to delanni/kibana that referenced this pull request Jan 11, 2024
…tity flyouts (elastic#174438)

This PR adds the Asset Criticality selector to both the new User and
Host expandable flyouts

User flyout:
<img width="770" alt="Screenshot 2024-01-08 at 12 17 57"
src="https://github.com/elastic/kibana/assets/2423976/6c07de00-4863-47ed-83f5-67aec6354585">

Host flyout:
<img width="749" alt="Screenshot 2024-01-08 at 12 18 09"
src="https://github.com/elastic/kibana/assets/2423976/815ed517-954b-4ab5-9ccf-5f4ca28ec5c2">


Part of [elastic#8148](elastic/security-team#8148)
and the parent epic
[elastic#4208](elastic/security-team#4208)

---------

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port of appStatus from 4.x branch
4 participants