[ML] Convert anomalies table to EUI / React #19352
Conversation
peteharverson
added
review
v7.0.0
v6.4.0
:ml
Feature:ml-results
|
Pinging @elastic/ml-ui |
💔 Build Failed |
| } | ||
| } | ||
| } else { | ||
| if (mouseOverRecord !== undefined) { |
There was a problem hiding this comment.
nit: This could be } else if (mouseOverRecord !== undefined) {.
| }; | ||
|
|
||
| render() { | ||
| if (this.props.tableData === undefined || |
There was a problem hiding this comment.
const { timefilter, tableData, filter } = this.props; could be added to the beginning of the render() method so in the code below this.props wouldn't be necessary all the time. This would make it clearer which props are used in the render() method. See section 4 in this article with more details about this pattern https://medium.com/walmartlabs/make-your-react-components-pretty-a1ae4ec0f56e
There was a problem hiding this comment.
I like this pattern. as well as making it clear what props are being used in the function, it also cleans up the code throughout the function.
| * a filter on this entity. | ||
| */ | ||
| export function EntityCell({ entityName, entityValue, filter }) { | ||
| const valueText = entityName !== 'mlcategory' ? entityValue : `mlcategory ${entityValue}`; |
There was a problem hiding this comment.
nit, entityName !== 'mlcategory' should be wrapped in parenthesis
| import { formatValue } from 'plugins/ml/formatters/format_value'; | ||
|
|
||
| const TIME_FIELD_NAME = 'timestamp'; | ||
| const INFLUENCERS_LIMIT = 5; |
There was a problem hiding this comment.
nit: this is defined in the same way in anomaly_table.js, it could be moved to a common constants file or passed down in the component tree from the parent.
| }; | ||
|
|
||
| render() { | ||
| const button = ( |
There was a problem hiding this comment.
Like in the previous comment, the used props could be destructured like const { ... } = this.props; too.
| field: 'severity', | ||
| name: `${(isAggregatedData === true) ? 'max ' : ''}severity`, | ||
| render: (score) => { | ||
| return ( |
There was a problem hiding this comment.
these render functions are a bit inconsistent, the first omits the return but the others don't.
Personally I don't thing the return is necessary if it's just returning a single component, as in the first column.
| return anomaly.rowId === rowId; | ||
| }); | ||
|
|
||
| return matching === undefined; |
There was a problem hiding this comment.
nit, similarly to ternary expressions, we should wrap conditions like this.
i.e. (matching === undefined)
| if (itemIdToExpandedRowMap[item.rowId]) { | ||
| delete itemIdToExpandedRowMap[item.rowId]; | ||
| } else { | ||
| const examples = item.entityName === 'mlcategory' ? |
There was a problem hiding this comment.
nit, should be (item.entityName === 'mlcategory')
| if (expandButton.length > 0) { | ||
| const rowId = expandButton.attr('data-row-id'); | ||
| mouseOverRecord = this.props.tableData.anomalies.find((anomaly) => { | ||
| return anomaly.rowId === rowId; |
There was a problem hiding this comment.
nit, should be (anomaly.rowId === rowId)
| }; | ||
|
|
||
| render() { | ||
| if (this.props.tableData === undefined || |
There was a problem hiding this comment.
I like this pattern. as well as making it clear what props are being used in the function, it also cleans up the code throughout the function.
|
|
||
| if (examples !== undefined && examples.length > 0) { | ||
| examples.forEach((example, index) => { | ||
| const title = index === 0 ? 'category examples' : ''; |
There was a problem hiding this comment.
nit, should be (index === 0)
| .then((resp) => { | ||
| // Prefix each of the terms with '+' so that the Elasticsearch Query String query | ||
| // run in a drilldown Kibana dashboard has to match on all terms. | ||
| const termsArray = _.map(resp.terms.split(' '), (term) => { return '+' + term; }); |
There was a problem hiding this comment.
could be written as
const termsArray = resp.terms.split(' ').map(term => `+${term}`)
| // Need to encode the _a parameter in case any entities contain unsafe characters such as '+'. | ||
| let path = chrome.getBasePath(); | ||
| path += '/app/ml#/timeseriesexplorer'; | ||
| path += '?_g=' + _g; |
There was a problem hiding this comment.
nit, should be using template literals
| const indexPatterns = getIndexPatterns(); | ||
|
|
||
| const job = mlJobService.getJob(this.props.anomaly.jobId); | ||
| const categorizationFieldName = job.analysis_config.categorization_field_name; |
There was a problem hiding this comment.
there is a low risk here that mlJobService.getJob will return undefined which will cause this line to throw an error
| } | ||
| }); | ||
|
|
||
| $timeout(() => { |
There was a problem hiding this comment.
same comment about $evalAsync
| } | ||
| }); | ||
|
|
||
| $timeout(() => { |
There was a problem hiding this comment.
$evalAsync should probably be used in situations like this.
peteharverson
force-pushed
the
ml-anomalies-table-react
branch
from
ce82449
to
ab3695e
Compare
💔 Build Failed |
peteharverson
force-pushed
the
ml-anomalies-table-react
branch
from
ab3695e
to
bc94853
Compare
💚 Build Succeeded |
* [ML] Convert anomalies table to EUI / React * [ML] Edits to EUI / React anomalies table after review
* [ML] Convert anomalies table to EUI / React * [ML] Edits to EUI / React anomalies table after review
Converts the anomalies table, as used on the Single Metric Viewer and Anomaly Explorer to EUI / React.
Functionally the table is the same, but is now built from the EUI components.
Existing Angular based table:
New EUI / React table:
Addresses anomalies table and anomalies table service items in the components list of #18374. Note the interval and severity controls, and the parent pages (Angular controller based), will be converted to EUI in future PRs.