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

Convert status page to EUI #21491

Merged
merged 2 commits into from
Aug 9, 2018
Merged

Conversation

joshdover
Copy link
Contributor

@joshdover joshdover commented Jul 31, 2018

Fixes #20001

This converts Kibana's status page to elastic-ui / React. Shoutout to @snide for the design heavy lifting. This also involved some minor changes to the status API on the server to faciliate making some of the data easier to work with on the frontend.

  • Add back the overall status to the top of the page (icon?)
  • Add back the row color coding in the table
  • Test new components (especially StatusTable)
  • Add test coverage to data fetching and formatting
  • Cleanup the server code to remove unneeded fields (eg. icon)
  • Error handling and display
  • Remove the old implementation
  • Update other usages (if any exist) of the status API

Screenshot

image

@joshdover joshdover added the WIP Work in progress label Jul 31, 2018
@elasticmachine
Copy link
Contributor

💔 Build Failed

</div>
</footer>
<div class="container">
<status-app build-num="{{ui.buildInfo.num}}" build-sha="'{{ui.buildInfo.sha}}'" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might be a better way of delivering this info the the React components?

@@ -0,0 +1 @@
{"version": "1.2.3"}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure where these came from...

@elasticmachine
Copy link
Contributor

💔 Build Failed

@joshdover joshdover force-pushed the status-page-eui branch 2 times, most recently from e49200d to 5372dec Compare August 1, 2018 20:15
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@joshdover joshdover added Team:Operations Team label for Operations Team review v7.0.0 v6.4.0 and removed WIP Work in progress labels Aug 3, 2018
@tylersmalley tylersmalley added v6.5.0 and removed v6.4.0 labels Aug 3, 2018
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@joshdover joshdover requested a review from tylersmalley August 6, 2018 14:42
@snide
Copy link
Contributor

snide commented Aug 7, 2018

Note that although this PR includes SCSS. It is not blocked by #21656. The single line of scss in this PR is safe from autoprefixer. https://caniuse.com/#feat=viewport-units

Copy link
Contributor

@tylersmalley tylersmalley left a comment

Choose a reason for hiding this comment

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

Just a couple questions, but LGTM. Great work!


<EuiSpacer />

<EuiPageContent grow={0}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Does EuiPageContent have a grow attribute?

Copy link
Contributor

Choose a reason for hiding this comment

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

No. This can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This gets passed to the child div, I added this to remove this extra whitespace at the end of the content:

image

@@ -62,7 +62,7 @@ export const all = [
id: 'disabled',
title: 'Disabled',
severity: -1,
icon: 'toggle-off',
uiColor: 'subdued',
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does subdued come from? I am not seeing it as an available color

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, originally I was passing this through to EuiIcon which has this color. Will change to default.

@tylersmalley
Copy link
Contributor

@jbudz mind taking a look?

state: this.state,
icon: states.get(this.state).icon,
message: this.message,
state: {
Copy link
Member

Choose a reason for hiding this comment

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

/cc @elastic/kibana-monitoring any idea if this is going to impact the beats module or status poller?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only usage I've found besides the status page / checking logic itself is the api_debug CLI dev tool in xpack. It doesn't do anything with the data back from the API besides dump it into the console and I've confirmed it still works.

Dropped in Monitoring's slack to confirm, but I think we will be good to go.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

metrics: PropTypes.arrayOf(MetricPropType).isRequired
};

export default MetricTiles;
Copy link
Member

Choose a reason for hiding this comment

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

nit: styleguide convention for no default exports, guess our ci task to lint didn't catch 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.

Hmm yeah it's passing lint. We have almost 1k export defaults, but happy to change it if this is the direction we want to move to.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@tylersmalley tylersmalley Aug 9, 2018

Choose a reason for hiding this comment

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

@spalger any idea why this wasn't caught by the linter? It doesn't appear to be ignored in https://github.com/elastic/kibana/blob/master/.eslintrc.js#L51

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That rule actually looks like it's allowing default exports, not preventing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably need to cut a new issue for changing all the existing ones. I'll make sure this PR is good though 👍

@joshdover joshdover merged commit 33c6ade into elastic:master Aug 9, 2018
@joshdover joshdover deleted the status-page-eui branch August 9, 2018 17:26
@joshdover joshdover restored the status-page-eui branch August 9, 2018 17:32
joshdover added a commit to joshdover/kibana that referenced this pull request Aug 9, 2018
* Convert the status_page plugin to EUI

* Fix uiColor for disabled state
epixa added a commit to epixa/kibana that referenced this pull request Aug 10, 2018
joshdover added a commit that referenced this pull request Aug 13, 2018
* Convert status page to EUI (#21491)

* Convert the status_page plugin to EUI

* Fix uiColor for disabled state

* Unbreak status API
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review Team:Operations Team label for Operations Team v6.5.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants