[One Discover] Add logic to customise column header in Discover via contextual exper…#226039
Conversation
|
Pinging @elastic/obs-ux-logs-team (Team:obs-ux-logs) |
src/platform/plugins/shared/discover/public/application/main/hooks/grid_customisations/logs.tsx
Show resolved
Hide resolved
logeekal
left a comment
There was a problem hiding this comment.
Hey @achyutjhunjhunwala , it will be great if you can add a demo of how this change changes UI. It helps to get some context regarding the change.
Thanks.
jughosta
left a comment
There was a problem hiding this comment.
Thanks for working on this new extension! Left some suggestions in comments.
src/platform/packages/shared/kbn-unified-data-table/src/components/data_table_column_header.tsx
Outdated
Show resolved
Hide resolved
src/platform/packages/shared/kbn-unified-data-table/src/components/data_table_column_header.tsx
Outdated
Show resolved
Hide resolved
src/platform/packages/shared/kbn-unified-data-table/src/components/data_table_column_header.tsx
Outdated
Show resolved
Hide resolved
src/platform/packages/shared/kbn-unified-data-table/src/components/data_table_columns.tsx
Outdated
Show resolved
Hide resolved
...file_providers/observability/logs_data_source_profile/accessors/get_column_configuration.tsx
Outdated
Show resolved
Hide resolved
...le_providers/observability/traces_data_source_profile/accessors/get_column_configuration.tsx
Outdated
Show resolved
Hide resolved
...le_providers/observability/traces_data_source_profile/accessors/get_column_configuration.tsx
Show resolved
Hide resolved
src/platform/plugins/shared/discover/public/context_awareness/types.ts
Outdated
Show resolved
Hide resolved
@logeekal Sorry, i missed to post it here. I added it in the original linked ticket https://github.com/elastic/logs-dev/issues/187#issuecomment-3024117125 |
tonyghiani
left a comment
There was a problem hiding this comment.
Codewise looks good to me and Julia already commented about improvements, it would be great to get a quick check on the copies.
@tonyghiani Thank you so much for the feedback and review. The copy has already been verified in the linked original ticket here - https://github.com/elastic/logs-dev/issues/187#issuecomment-3024167718 |
...e_providers/observability/traces_data_source_profile/accessors/get_columns_configuration.tsx
Outdated
Show resolved
Hide resolved
| <FormattedMessage | ||
| id="discover.unifiedDataTable.tableHeader.logsContext.sourceFieldIconTooltip" | ||
| defaultMessage="Displays the most relevant resource identifiers like:{br}{br}service.name{br}host.name{br}kubernetes.pod.name{br}etc.{br}{br}followed by the log or error message.{br}if no message fields are present, it shows the full document instead" | ||
| values={{ br: <br /> }} |
There was a problem hiding this comment.
I tested it locally because the screenshots you shared didn’t quite match the content, just wanted to double check how it looked in the UI.
In the logs one, for example, it feels a bit off: there are line breaks, some sentences start without a capital letter after a dot, and the final dot is missing.

There was a problem hiding this comment.
@iblancof The text was changed after the demo session to the team last week.I will update the screenshot in the description of this PR now.
The text and design both are indeed intended.
Line breaks are required here to make this list looks more readable.
The whole thing is a single sentence, just formatted like this.
Good catch on the period at the end of the sentence, and the missing capitalisation at start of the sentence. i will add that
There was a problem hiding this comment.
Thanks for sharing the context and making those small tweaks!
There was a problem hiding this comment.
Agree with @iblancof that the tooltip content looks off. Even after the latest changes. Too many line breaks reduces readability of it.
Also instead of:
servicen.name
host.name
kubernetes.pod.name
etc
we could rather show it as a list:
- servicen.name
- host.name
- kubernetes.pod.name
- etc
There was a problem hiding this comment.
Or rewrite it as 2 paragraphs:
Displays the most relevant resource identifiers (service.name, host.name, kubernetes.pod.name, etc) followed by the log or error message.
If no message-fields are present, it shows key:value pairs for the available fields.
There was a problem hiding this comment.
@jughosta This design has been discussed multiple times and already approved by the Product team (cc: @LucaWintergerst)
Too many line breaks may be makes it difficult to read it in the code, but the intention here is to make it more readable to end user.
I like the suggestion to make it as a list adding the hyphens.
Unfortunately this could have been better handled inside a EuiPopover but given EUI guidenlines, we cannot use a Popover for onHover triggers.
There was a problem hiding this comment.
Please check the description for updated Screenshot
logeekal
left a comment
There was a problem hiding this comment.
Thanks for reminder. I did the desk testing of Security solution and changes look good.
jughosta
left a comment
There was a problem hiding this comment.
Thanks for the changes!
...m/packages/shared/kbn-unified-data-table/src/components/data_table_summary_column_header.tsx
Outdated
Show resolved
Hide resolved
...m/packages/shared/kbn-unified-data-table/src/components/data_table_summary_column_header.tsx
Outdated
Show resolved
Hide resolved
...m/packages/shared/kbn-unified-data-table/src/components/data_table_summary_column_header.tsx
Outdated
Show resolved
Hide resolved
...m/packages/shared/kbn-unified-data-table/src/components/data_table_summary_column_header.tsx
Outdated
Show resolved
Hide resolved
| ...column, | ||
| display: ( | ||
| <DataTableSummaryColumnHeaderLogsContext | ||
| columnDisplayName={column.displayAsText} |
There was a problem hiding this comment.
It will be controlled then by another logic
| columnDisplayName={column.displayAsText} |
If the prop is removed, then it would use the new default value which you defined in src/platform/packages/shared/kbn-unified-data-table/src/components/data_table_summary_column_header.tsx
There was a problem hiding this comment.
May be this is the intended behaviour.
How i see this is, the column get passed as a prop to this customisation which is used to render the column header. Now i want to keep the existing logic as much a possible and only apply customisation where required.
Meaning the columnDisplayNameis currently controlled via this function getColumnDisplayName. If tomorrow someone does some changes to this generic function, every other usage of this customisation would automatically inherit it.
whereas the default handling logic would handle situation where a generic customisation decided to completely remove the name, which wouldn't be ideal
|
|
||
| return ( | ||
| <UnifiedDataTableSummaryColumnHeader | ||
| columnDisplayName={columnDisplayName} |
There was a problem hiding this comment.
| columnDisplayName={columnDisplayName} |
There was a problem hiding this comment.
same as above
| ...column, | ||
| display: ( | ||
| <DataTableSummaryColumnHeaderTracesContext | ||
| columnDisplayName={column.displayAsText} |
There was a problem hiding this comment.
| columnDisplayName={column.displayAsText} |
There was a problem hiding this comment.
same as above
|
|
||
| return ( | ||
| <UnifiedDataTableSummaryColumnHeader | ||
| columnDisplayName={columnDisplayName} |
There was a problem hiding this comment.
| columnDisplayName={columnDisplayName} |
| <FormattedMessage | ||
| id="discover.unifiedDataTable.tableHeader.logsContext.sourceFieldIconTooltip" | ||
| defaultMessage="Displays the most relevant resource identifiers like:{br}{br}service.name{br}host.name{br}kubernetes.pod.name{br}etc.{br}{br}followed by the log or error message.{br}if no message fields are present, it shows the full document instead" | ||
| values={{ br: <br /> }} |
There was a problem hiding this comment.
Agree with @iblancof that the tooltip content looks off. Even after the latest changes. Too many line breaks reduces readability of it.
Also instead of:
servicen.name
host.name
kubernetes.pod.name
etc
we could rather show it as a list:
- servicen.name
- host.name
- kubernetes.pod.name
- etc
| <FormattedMessage | ||
| id="discover.unifiedDataTable.tableHeader.logsContext.sourceFieldIconTooltip" | ||
| defaultMessage="Displays the most relevant resource identifiers like:{br}{br}service.name{br}host.name{br}kubernetes.pod.name{br}etc.{br}{br}followed by the log or error message.{br}if no message fields are present, it shows the full document instead" | ||
| values={{ br: <br /> }} |
There was a problem hiding this comment.
Or rewrite it as 2 paragraphs:
Displays the most relevant resource identifiers (service.name, host.name, kubernetes.pod.name, etc) followed by the log or error message.
If no message-fields are present, it shows key:value pairs for the available fields.
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
async chunk count
History
|
…ontextual exper… (elastic#226039) closes elastic/logs-dev#187 This PR adds an extension point to update Discover Column Headers in general via Contextual Profiles. For this particular PR, we only add icons for the summary column, but this PR would enable users to completely control the column header.
closes https://github.com/elastic/logs-dev/issues/187
This PR adds an extension point to update Discover Column Headers in general via Contextual Profiles. For this particular PR, we only add icons for the summary column, but this PR would enable users to completely control the column header.
Tooltip with Contextual Experience - Logs
Tooltip with Contextual Experience - Traces
Tooltip without Contextual Experience - Default