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

[ML] Data Frames Summary Stats Bar #43986

Merged
merged 8 commits into from
Aug 27, 2019
Merged

[ML] Data Frames Summary Stats Bar #43986

merged 8 commits into from
Aug 27, 2019

Conversation

alvarezmelissa87
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 commented Aug 26, 2019 •
edited
Loading

Summary

  • Move stats bar into it's own component for reuse in jobs list and data frames list
  • Use new stats bar component in job management list
  • Add stats bar to Data Frame management list
    • Moves data frame fetching into parent component so data can be accessed by DataFrameList component and StatsBar component.
    • Only displays failed jobs if it is a non-zero value - consistent with job management list
    • Displays Total, Batch, Continuous, Started, and Failed (if any)

image

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

- [ ] This was checked for breaking API changes and was labeled appropriately
- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately

@alvarezmelissa87 alvarezmelissa87 requested review from a team as code owners August 26, 2019 13:45
@alvarezmelissa87
Copy link
Contributor Author

Does the data displayed in the DF stats bar make sense? cc @peteharverson

walterra
walterra reviewed Aug 26, 2019
View reviewed changes
Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

Overall looks good, just added a few questions! And just a general thought: IMHO is looks fine to get it done for this PR, but maybe in a follow up we could move the span elements with custom CSS to EUI using flex for example.

x-pack/legacy/plugins/ml/common/types/common.ts Outdated Show resolved Hide resolved
x-pack/legacy/plugins/ml/public/components/stats_bar/_stat.scss
@@ -0,0 +1,7 @@
.stat {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest to make the class names prefixed with ml and not nested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good - this will likely be fixed by the follow up to switch to using the Eui Stat component.

...ml/public/data_frame/pages/transform_management/components/transform_list/transform_list.tsx Show resolved Hide resolved
@elasticmachine
Copy link
Contributor

💔 Build Failed

@walterra
Copy link
Contributor

walterra commented Aug 26, 2019 •
edited by alvarezmelissa87
Loading

I tested this locally and the stats bar didn't pick up the necessary styles - could you please have a look and confirm it's working for you - could it be that you missed adding/pushing another _index.scss to the PR that includes the classes?

Looks like I needed to remove an import for styles that was deleted from jobs_list/_index.scss but didn't catch it until I did a full clean and reboostrap. Good catch 👍 8f2a9a6
cc @walterra

@ryankeairns
Copy link
Contributor

The presentation of these stats could be quickly enhanced by using the EuiStat component: https://elastic.github.io/eui/#/display/stat

ryankeairns
ryankeairns reviewed Aug 26, 2019
View reviewed changes
x-pack/legacy/plugins/ml/public/components/stats_bar/_stats_bar.scss
height: 42px;
padding: 14px;
background-color: $euiColorLightestShade;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You could also drop these styles by using EuiStat inside of an EuiFlexGroup.

@alvarezmelissa87
Copy link
Contributor Author

@ryankeairns - thanks so much for taking a look!
In this PR, I'm inclined not to change the design as IMO that’s out of scope to get it in for 7.4. Though updating this to use the suggested EUI stat component and Flex sounds like a good follow-up.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

peteharverson
peteharverson approved these changes Aug 27, 2019
View reviewed changes
Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

LGTM

jgowdyelastic
jgowdyelastic approved these changes Aug 27, 2019
View reviewed changes
Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

LGTM

@alvarezmelissa87 alvarezmelissa87 merged commit 3cc4653 into elastic:master Aug 27, 2019
@alvarezmelissa87 alvarezmelissa87 mentioned this pull request Aug 27, 2019
Merged
@alvarezmelissa87 alvarezmelissa87 deleted the ml-df-status-bar branch August 27, 2019 15:58
alvarezmelissa87 added a commit that referenced this pull request Aug 27, 2019
@alvarezmelissa87
[ML] Data Frames Summary Stats Bar (#43986) (#44111)
ca83624 
* move stat component out of jobStatsBar for reuse

* create transformStatsBar component

* add transformStatsBar to DF page

* update tests

* move create statsBar component for reuse

* move stat component into statsBar component

* move statsBar related types to stats_bar dir

* rename scss file. remove unnecessary import
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryankeairns ryankeairns ryankeairns left review comments

@walterra walterra walterra left review comments

@jgowdyelastic jgowdyelastic jgowdyelastic approved these changes

@peteharverson peteharverson peteharverson approved these changes

Assignees
No one assigned
Labels
Feature:Transforms ML transforms release_note:enhancement v7.4.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@alvarezmelissa87 @elasticmachine @walterra @ryankeairns @peteharverson @jgowdyelastic