-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] Convert anomalies controls to EUI / React #19856
[ML] Convert anomalies controls to EUI / React #19856
Conversation
Pinging @elastic/ml-ui |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just added a few nit picks
}; | ||
} | ||
// Restore the checked setting from the state. | ||
const mlCheckboxShowChartsService = this.props.mlCheckboxShowChartsService; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit. could be added as a class variable. e.g. this.mlCheckboxShowChartsService = ...
so it doesn't then have to be read out of the props again in the onChange
function
function optionValueToInterval(value) { | ||
// Builds the corresponding interval object with the required display and val properties | ||
// from the specified value. | ||
const option = OPTIONS.find(opt => opt.value === value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit. conditions should be wrapped in braces.
const option = OPTIONS.find(opt => (opt.value === value));
|
||
import template from './select_severity.html'; | ||
import 'plugins/ml/components/controls/controls_select'; | ||
const OPTIONS = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these colours are hardcoded a number if times throughout our code base.
It would be nice to move them to a constants file.
getSeverityColor
also exists to return a colour based on score, this could also be used here to avoid the hard coding of the hex values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. IMHO no need to port these mocha tests 1:1 in a follow up because they were just instantiating the directive and testing against the default scope which would be the equivalent of using PropTypes
.
💚 Build Succeeded |
💚 Build Succeeded |
* [ML] Convert anomalies controls to EUI / React * [ML] Edits to anomaly controls following review
* [ML] Convert anomalies controls to EUI / React * [ML] Edits to anomaly controls following review
Converts the three controls for the Anomalies table and charts in the Anomaly Explorer and Single Metric Viewer to EUI / React.
Before:
After:
The units tests for the old Angular based controls have been removed, and will be replaced by Jest tests in a future PR.
Addresses items in the Controls section of #18374