-
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] Anomaly Detection: Consolidate severity colors #204803
base: main
Are you sure you want to change the base?
Conversation
Pinging @elastic/ml-ui (:ml) |
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
💔 Build Failed
Failed CI StepsMetrics [docs]Module Count
Async chunks
History
cc @walterra |
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.
Overall LGTM, left some notes
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.
Suggestion to update the function description above
Returns a severity RGB color (one of critical, major, minor, warning, low or unknown) |
unknown
instead of blank
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.
Thanks for spotting this, updated in 98d1a09.
.range([ | ||
ML_SEVERITY_COLORS.LOW, | ||
ML_SEVERITY_COLORS.WARNING, | ||
ML_SEVERITY_COLORS.MINOR, |
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.
MINOR
and MAJOR
are slightly different here (than #ffdd00 and #ff7e00), but I assume this is intentional?
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.
Again thanks for your attention to detail! Discussed with Pete and we can't remember if this was intentional or an oversight originally, so we'll move forward with the consolidation to make it the same color.
}, | ||
|
||
'&.low': { | ||
fill: mlColors.lowWarning, | ||
fill: ML_SEVERITY_COLORS.LOW, |
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.
Just noting, it might be intentional as well but ML_SEVERITY_COLORS.LOW
is different than mlColors.lowWarning
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.
Same as the other, comment above, not sure if this was intentional or oversight way back.
Summary
Part of #140695.
We defined the hex codes for anomaly detection severity colors in several places. This PR consolidates this and makes sure the hex codes are defined in just one place. Note this PR doesn't change any of the colors or styling related to anomaly detection, it is pure refactoring to make future updates to these colors more easy.
BLANK
toUNKNOWN
to be in line with severity labels.ML_SEVERITY_COLORS.*
in test assertions so future color changes won't need updating every assertion.ML_SEVERITY_COLORS
. Therefor the SCSS based severity colors get removed in this PR.Checklist
release_note:*
label is applied per the guidelines