-
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
[Discover] Add log level badge cell renderer for logs profile #188281
[Discover] Add log level badge cell renderer for logs profile #188281
Conversation
/ci |
⏳ Build in-progress, with failures
Failed CI StepsTest Failures
Historycc @davismcphee |
bbfacef
to
8e62303
Compare
/ci |
/ci |
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
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.
Investigations code owner changes! 👍🏾
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.
Awesome! More colors on Discover page 🙂
src/plugins/discover/public/components/data_types/logs/log_level_badge_cell.tsx
Show resolved
Hide resolved
src/plugins/discover/public/components/data_types/logs/log_level_badge_cell.tsx
Show resolved
Hide resolved
...ontext_awareness/profile_providers/logs_data_source_profile/accessors/get_cell_renderers.tsx
Outdated
Show resolved
Hide resolved
data-test-subj={`logLevelBadgeCell-${coalescedValue ?? 'unknown'}`} | ||
css={badgeCss} | ||
> | ||
{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.
I think for some badges we might want to use white text instead of dark. For example for emerg
badge here:
Or is it fine with the default dark color text regardless of current badge background color?
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.
I thought the same thing, although it currently matches the mockups and it seems EUI's util that is supposed to do this automatically determined the text should be dark (I tested with an extra dark colour to confirm it renders white in that case). If we want to do this, we'll need to manually decide which ones should have white text, so I'll leave it to @MichaelMarcialis to determine if we should.
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.
I merged this PR to get the feature in, but we can make followup changes around this if needed based on @MichaelMarcialis' feedback.
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 the ping, @jughosta and @davismcphee! Yes, the badges' black text color used in the mockups is correct and passes the AA WCAG accessibility standard for normal text. Switching to white will actually cause the text within the badge to fail this contrast check, even in the emerg
badge.
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 confirming @MichaelMarcialis!
@@ -7,3 +7,4 @@ | |||
*/ | |||
|
|||
export { getRowIndicatorProvider } from './get_row_indicator_provider'; | |||
export { getCellRenderers } from './get_cell_renderers'; |
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.
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.
Definitely doable, although I'd prefer to save it for a followup if we do since it'll require some refactoring of the current log level chip component. @elastic/obs-ux-logs-team WDYT about this suggestion to update the log level chip to use the new log badge component for consistency? If we all agree, I'll create a separate issue to implement this as a followup.
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.
Always in favour of visual consistency, +1 for updating it and make them look the same 👌
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.
Great! I created a followup issue to take care of it here: #188553.
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 👍
Summary
This PR adds a log level badge cell render for
log.level
andlog_level
fields when the logs data source profile is active in Discover:Resolves #186567.
Checklist
For maintainers