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

[ML] Migrate bucket span estimator button to EUI/React. #19045

Merged
merged 5 commits into from
May 17, 2018

Conversation

walterra
Copy link
Contributor

@walterra walterra commented May 14, 2018

This migrates the bucket span estimator button to use React/EUI.

The existing mlBucketSpanEstimator is refactored to use React without ngReact to have more detailed control in regards to the $scope being transformed to a trimmed down props object with only the information necessary for the React component. This also allows more control in regards to the $scope attributes being watched. Instead of custom styles and classes this now uses the options available for EuiButton. However, there were some minimal overrides necessary to the EUI styles to replicate the narrower height and padding to fit within the input element.

image

Part of #18374.

@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

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. Just a question about whether we should still be flagging this up as an experimental feature.

return (
<div className="bucket-span-estimator">
<EuiToolTip
content="Experimental feature for estimating bucket span."
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this warning about it being an experimental feature is still needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

We are adding in k-api tests shortly. I'm not sure how far we'll get in terms of depth of coverage in this release, so let's keep as experimental till we have a stronger set of data scenario tests.

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

@walterra walterra force-pushed the ml-eui-tooltips-2 branch from fd3762a to ea5e2c5 Compare May 15, 2018 15:35
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

// watch for these changes
$scope.$watch('formConfig.agg.type', updateButton, true);
$scope.$watch('jobStateWrapper.jobState', updateButton, true);
$scope.$watch('JOB_STATE', updateButton, true);
Copy link
Member

Choose a reason for hiding this comment

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

JOB_STATE is a constant and so doesn't need to be watched.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@walterra
Copy link
Contributor Author

jenkins test this

@elasticmachine
Copy link
Contributor

💔 Build Failed

@walterra
Copy link
Contributor Author

jenkins test this

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@walterra walterra merged commit a9d4732 into elastic:master May 17, 2018
@walterra walterra deleted the ml-eui-tooltips-2 branch May 17, 2018 07:26
walterra added a commit to walterra/kibana that referenced this pull request May 17, 2018
This migrates the bucket span estimator button to use React/EUI. The existing mlBucketSpanEstimator is refactored to use React without ngReact to have more detailed control in regards to the $scope being transformed to a trimmed down props object with only the information necessary for the React component. This also allows more control in regards to the $scope attributes being watched. Instead of custom styles and classes this now uses the options available for EuiButton. However, there were some minimal overrides necessary to the EUI styles to replicate the narrower height and padding to fit within the input element.
walterra added a commit that referenced this pull request May 17, 2018
)

This migrates the bucket span estimator button to use React/EUI. The existing mlBucketSpanEstimator is refactored to use React without ngReact to have more detailed control in regards to the $scope being transformed to a trimmed down props object with only the information necessary for the React component. This also allows more control in regards to the $scope attributes being watched. Instead of custom styles and classes this now uses the options available for EuiButton. However, there were some minimal overrides necessary to the EUI styles to replicate the narrower height and padding to fit within the input element.
@walterra walterra changed the title [ML] Migrate bucket span estimator button to React/EUI. [ML] Migrate bucket span estimator button to EUI/React. May 17, 2018
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

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.

5 participants