elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.8k stars 8.19k forks source link

[OneDiscover][UnifiedDocViewer] Auto-pin default selected fields #188404

Closed kertal closed 1 month ago

kertal commented 3 months ago

📓 Summary:

Follow up of #186271

When expanding documents, the default selected fields of a contextual awareness profile should also be pinned by default (unless the user already has manually changed the selection)

Mockup: Context-Aware_UX_For_Logs___Version_5__In_Progress__–_Figma

✔️Acceptance criteria:

elasticmachine commented 3 months ago

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

kertal commented 3 months ago

@MichaelMarcialis is this correct?

MichaelMarcialis commented 2 months ago

I believe this description is correct. CCing @ninoslavmiskovic as well to confirm.

Additionally, we should probably ask ourselves whether this behavior should be consistent across all Discover scenarios, not just the logs-aware cases. Should we always auto-pin fields in the document viewer whenever a user has added those fields to their Discover data table (assuming they haven't made any manual pinnings on their own)? That might be a nice touch for consistency.

For example, if I'm also working with non-logs data and I add fields to my Discover data table, opening the document viewer could have those fields auto-pinned in the flyout because Discover now knows I'm interested in them. Thoughts?

davismcphee commented 2 months ago

I like the idea of making it consistent, although the UX might be a bit tricky to get right. We currently store pinned fields in local storage by data view ID or index pattern hash for ES|QL. We could check if any are stored when expanding a row and default the pinned columns if there are none, but would those defaults now be stored too or only temporary? If they're only temporary, would they automatically unpin when the user pins another field, or maybe they get stored at that point? If they're temporary and don't get automatically unpinned or stored when pinning another field, how would a user pin a default field so that it gets stored in local storage? Just some things that come to mind when thinking about it, but in general I like the idea of a consistent behaviour.

MichaelMarcialis commented 2 months ago

We could check if any are stored when expanding a row and default the pinned columns if there are none, but would those defaults now be stored too or only temporary?

In that case, I would assume temporary would be best.

If they're only temporary, would they automatically unpin when the user pins another field, or maybe they get stored at that point?

I think simply storing at the point forward would make the most sense. So all auto-pinned fields would then get stored, plus the additional pinning changes that the user has indicated (some of which may be pinning additional fields or unpinning auto-pinned ones).

kertal commented 2 months ago

For example, if I'm also working with non-logs data and I add fields to my Discover data table, opening the document viewer could have those fields auto-pinned in the flyout because Discover now knows I'm interested in them. Thoughts?

my 2 cents. I like simplicity . dear user, your selected fields are auto-pinned in the doc viewer, but they are not stored until you change this pinning sounds simple

jughosta commented 2 months ago

I like the idea of making it consistent and simply show all currently selected fields (in the order of selection) at the top in the Doc Viewer. Although I would not mix it with "pin" functionality.

We could extend EuiDataGrid with "divider" rows (spanning all columns) where we could add a header for the following rows in that part of the grid. Similarly to:

FieldValue
Pinned fields
field1value1
......
Selected fields
fieldAvalueA
......
Available fields
fieldNvalueN
......
kertal commented 2 months ago

I like the idea of making it consistent and simply show all currently selected fields (in the order of selection) at the top in the Doc Viewer. Although I would not mix it with "pin" functionality.

We could extend EuiDataGrid with "divider" rows (spanning all columns) where we could add a header for the following rows in that part of the grid. Similarly to:

Field Value Pinned fields field1 value1 ... ... Selected fields fieldA valueA ... ... Available fields fieldN valueN ... ...

QQ: if we would use divider rows, this would mean we would need to use the custom row renderer, and this would mean we would need to implement our own virtualization, right?

jughosta commented 2 months ago

@kertal Not necessarily. We could add those dividers to rows list and have a custom logic when rendering cells. Different styling for them we could achieve via rowClasses. The only problematic thing is how to merge cells inside such divider rows. A workaround could be to have shorter labels which would fit in the first cell of the row. Yeah, it's quite cumbersome but an option nevertheless.

kertal commented 2 months ago

@kertal Not necessarily. We could add those dividers to rows list and have a custom logic when rendering cells. Different styling for them we could achieve via rowClasses. The only problematic thing is how to merge cells inside such divider rows. A workaround could be to have shorter labels which would fit in the first cell of the row. Yeah, it's quite cumbersome but an option nevertheless.

it couldn definitely be an option, but it sounds like a lot of workarounds. For me auto-pinning selected fields unless user decided differently, and storing them if users change the selection sounds a more feasible way. I don't thing there's urgency to start implementing this, so I think this could wait for input by @ninoslavmiskovic, when returning from PTO

In this area of UI I think https://github.com/elastic/kibana/issues/188413 should be implemented first, anyway

ninoslavmiskovic commented 2 months ago

I like the idea of saving if they change the pinned fields. Would that be the same behavior when changing the default columns? If yes, then we will be consistent, which is good. If not, we should strive for consistency.

lukasolson commented 2 months ago

I like the idea of making it consistent and simply show all currently selected fields (in the order of selection) at the top in the Doc Viewer. Although I would not mix it with "pin" functionality.

Just wanted to give my +1 to this idea. I think it would also be helpful to show some sort of indicator alongside the selected columns to indicate that they are the currently selected columns.

My concern with using the "pin" functionality for this is that it becomes unclear to the user whether or not the pin functionality is synced with the selected columns - they might think "Oh, these are pinned because I have them selected in the table," then unpin one or pin a different field and then be confused as to why they're not being synced over to the data table selected columns.

MichaelMarcialis commented 1 month ago

Update from @ryankeairns during the Discover team sync:

Here’s what we settled on for the pinning conversation during the discovery team sync. Basically, don’t mess with pinning and certainly don’t mix selected fields with pinned fields. In the end, I suggested we simply add a basic filter/switch in the document flyout that allows the user to ‘Show only selected fields’. Then we dont mess with the alphabetical sort or things jumping around in order, either.

kertal commented 1 month ago

Closing in favor of #191536