Skip to content
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

[Lens] Transpose columns #89748

Merged
merged 105 commits into from
Mar 11, 2021
Merged

Conversation

flash1293
Copy link
Contributor

@flash1293 flash1293 commented Jan 29, 2021

Fixes #3613
Fixes #3620
Fixes #69450
Fixes #9585
Fixes #61489

Adds a way to split metric columns in a data table by one or multiple dimensions

Screenshot 2021-02-02 at 14 14 24

Considerations

  • Currently column width and "hide" is configured per metric column and is reflected for all transposed columns. Does this make sense?
  • Changed sorting slightly to always sort undefined values to the bottom (as they can occur in the middle of the dataset now)
  • Sorting is saved per transposed column and ignored if the column disappears due to changing data
  • Hiding a column is disabled from the settings popover for transposed columns as it would have no effect
  • The transposed metric columns are grouped by transposing key (allowing us to introduce multi-row headers with colspans later on to better show the hierarchy?
  • Filter values from transposed columns (part of the action popover)?
  • What happens for row click actions?
  • The inspector data is transposed as well - is that the right thing to do?
  • Right now both column split and row split are available from the start - this might be confusing because people will use the top most "empty" slot first, even if it's not the right thing for most situations.
    • Should we disable it until there is at least one metric in the rows group?
    • Should we hide it completely until there is at least one metric in the rows group?
    • Should we collapse it to make it less obvious?

flash1293 and others added 30 commits January 4, 2021 15:29
@flash1293
Copy link
Contributor Author

@MichaelMarcialis

I kept it to a single width per metric to make it less surprising by avoiding hidden state - if I save the width per actual column, it’s possible this column disappears depending on the data. I imagine most people resize a column to fit it to their data, but if there’s a separate width per value of the split column dimension, that’s very hard to do.

If the width is set to 140px for “css count of records”, and later on a dashboard a “html count of records” column appears, does it make sense to have it using the default width ?
I understand it’s a bit surprising the first time, but I imagine in daily use it’s more helpful

What do you think?

And about the column hiding - I’m currently working on a filter menu which is more helpful here I think

Eg you will be able to filter out css in the column popover which will effectively hide the column

I think that’s more in line with how “hide” is intended to be used

The reason to hide a metric is for example if a “top values” is sorted by it, but you don’t want to show it.

That’s not the same as hiding a column based on the value of the split dimension, filters are a better tool for that

My suggestion is basically keeping the current behavior for those two things. I agree with your other points, I will work on a fix them

The changes about the slider for selecting the number of terms and moving the “advanced” popover I would like to split out into a separate PR because they will affect all visualizations as this part of the UI is not specific to tables

@MichaelMarcialis
Copy link
Contributor

I kept it to a single width per metric to make it less surprising by avoiding hidden state - if I save the width per actual column, it’s possible this column disappears depending on the data. I imagine most people resize a column to fit it to their data, but if there’s a separate width per value of the split column dimension, that’s very hard to do.

If the width is set to 140px for “css count of records”, and later on a dashboard a “html count of records” column appears, does it make sense to have it using the default width ?
I understand it’s a bit surprising the first time, but I imagine in daily use it’s more helpful

What do you think?

Ah, I see your point about user-customized widths being lost as the data changes, when other columns come into replace previous ones. Using your example, let's assume "css count of records" is the first top value column, and is replaced by "html count of records" at a later time. Rather than attributing the column width state to "css count of records," can we not instead attribute that width state to the first top value column (regardless of what that column value is)? Doing so would prevent losing column width customizations and still meet with user expectations as to what will happen during the column resize interaction.

Otherwise, if that's not possible or if it would be excessively difficult to achieve, I think your current solution could be a fine alternative. Would it be possible to highlight the other affected columns during the interaction so the user is aware that the width will be affected in those columns as well?

And about the column hiding - I’m currently working on a filter menu which is more helpful here I think

Eg you will be able to filter out css in the column popover which will effectively hide the column

I think that’s more in line with how “hide” is intended to be used

The reason to hide a metric is for example if a “top values” is sorted by it, but you don’t want to show it.

That’s not the same as hiding a column based on the value of the split dimension, filters are a better tool for that

Thanks for that explanation. In that case, this makes sense to me.

Perhaps it would be also useful to the user to indicate that a particular dimension item is hidden from the table in the layer panel view. For example, maybe we could prepend a eyeClosed icon before the dimension item name. Of course that may interfere with the prepended color icon that may appear on dimension items in the upcoming dynamic color feature, but I suppose we can address that when we come to it. Thoughts?

Also, any chance we could just shorten the field label to be "Hide column"?

The changes about the slider for selecting the number of terms and moving the “advanced” popover I would like to split out into a separate PR because they will affect all visualizations as this part of the UI is not specific to tables

Sounds good. Let me know if you'd like me to open issues for those, if you haven't already.

@flash1293
Copy link
Contributor Author

flash1293 commented Mar 6, 2021

@MichaelMarcialis

Doing so would prevent losing column width customizations and still meet with user expectations as to what will happen during the column resize interaction

I thought about tying the width to the position instead of the value (1st column, 2st column of that type), and we can do that, but the following things prevented me from doing so:

  • it’s still a bit manual - what if you resize the first 5 transposed columns manually, then on a dashboard you get 7 columns? Also, the values in these columns are very often similar in size (because they are produced by the same metric), so I imagined people commonly want to give them the same width anyway. The first time a transposed column is resized, it’s definitely a bit surprising, but IMHO it would be pretty obvious what happened so the user learns quickly. I understand it’s depending on the use case, it’s hard to tell without rolling some version of the feature to test the waters.
  • It’s introducing a bunch of edge cases in transitions - if a custom width is set for a metric column, and then a split is configured, what should be the width of these columns? If various widths are configured and the column split is removed, what width should be used for the single metric column? We can definitely find answers to these questions, but that’s the kind of thing that’s introducing a lot of complexity in the code.
  • It's creating hidden state the user can't see or change in some cases which is something I want to avoid if possible. If the user configures the width of some columns, then these columns disappear, the state is still there (and might be used on dashboards if more columns appear there based on the data), but is unreachable for the user.

In my mind (and I should have explained my reasoning better, sorry for that), the current status wins all things considered (complexity, common user intentions, surprising behavior).

I don’t hold this opinion strongly though, so if you think the positional width per column is a better behavior considering these points, I’m happy to build that. Let me know what you think.

Would it be possible to highlight the other affected columns during the interaction so the user is aware that the width will be affected in those columns as well?

not without changes in EUI

@flash1293
Copy link
Contributor Author

flash1293 commented Mar 8, 2021

@MichaelMarcialis

Summary

Shared width of all columns of a certain metric

I think the current behavior, while the thing the user will expect the very first time, is still the best what we can do. See here for my reasoning #89748 (comment) If you disagree I'm happy to change this, but it will introduce new edge cases we will have to handle.

Can you elaborate on what you mean by "row click actions?" Not sure I'm following.

On a dashboard it's possible to configure a url drilldown which will add an "action" button for each row. The URL is built using the current row as context. I think it makes sense to offer the columns of the transposed table here

As I mention in a previous comment, I think we should change the order to have the row dimension appear before the column dimension (and handle the possibility of user controlled nesting in a separate issue).

Done

The designs use a dash as a dividing character between the consolidated column heading names, but those appear to be absent here. Would it be possible to add such a dividing character?

Added a dividing character

The designs intentionally removed the slider from "Number of values" field in "Top value" functions (as it is not useful for precision control and potentially confusing as 100 isn't the maximum limit), but it still remains here. Would it be possible to remove?
Would it be possible to move the "Advanced" buttons to the bottom left of the gray container to align more closely with the designs?

Created a separate issue here: #93912

Is "Group by" here just changing the dimension item order? If so, is this field necessary? Can we not just allow the users to manage this via the reorderable dimension items interface on the layer?

That was a bug, removed it

I was under the impression that the column hiding action would be managed via the column menu in the EuiDataGrid. Is that no longer the case?

Both is possible

In the column configuration flyout, there is a field for text alignment that doesn't appear to actually do anything. Text alignment was intentionally omitted from the designs for column configurations, and only offered as an option for row and metric dimension configurations. Is this inclusion intentional or a bug?

That was a bug as well

Perhaps it would be also useful to the user to indicate that a particular dimension item is hidden from the table in the layer panel view. For example, maybe we could prepend a eyeClosed icon before the dimension item name

We do this already:
Screenshot 2021-03-08 at 11 09 40

@flash1293
Copy link
Contributor Author

flash1293 commented Mar 8, 2021

@MichaelMarcialis

another case where I think changing all column widths at once is beneficial- if you do more than one column split, it’s very likely you will end up with a table using horizontal scroll. In this case data grid will give each column 100px. This is not enough for the full header in most cases. If we give each column their own width, the user would have to manually drag every column to the desired width (and the result would not be uniform, so it would be awkward looking). With the current implementation the user has just to resize the first column and the others will be “fixed” in the same way. In general I think there should be a “fit to data” mode, but that’s something for later down the road.

@flash1293
Copy link
Contributor Author

@elasticmachine merge upstream

@MichaelMarcialis
Copy link
Contributor

another case where I think changing all column widths at once is beneficial- if you do more than one column split, it’s very likely you will end up with a table using horizontal scroll. In this case data grid will give each column 100px. This is not enough for the full header in most cases. If we give each column their own width, the user would have to manually drag every column to the desired width (and the result would not be uniform, so it would be awkward looking). With the current implementation the user has just to resize the first column and the others will be “fixed” in the same way. In general I think there should be a “fit to data” mode, but that’s something for later down the road.

True. You're starting to win me over to your current implementation. My biggest concern is just giving users appropriate visual/audible feedback to let them know that this column resize action is affecting multiple columns. I'd say for the purposes of this PR, let's keep the resizing of columns the way you have it. As for the visual/audible feedback, if it needs to be addressed on the EUI side for EuiDataGrid, let's open an issue there to see if it's something that can be added in the near future. Would you prefer I write up the issue? Or would you care to do so, as you likely have more technical knowledge on what would be the desired implementation.

Copy link
Contributor

@MichaelMarcialis MichaelMarcialis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, @flash1293. Thanks for making those changes. I left a comment above about my suggestion for proceeding with the column resizing issue and a few comments below for some minor wording tweaks.

Additionally, I had two more questions/comments upon re-review of this PR:

Indicate when ordering is by grand total

Would it be possible to indicate when the "Rank by" fields are performing their ordering by a grand total of a metric? For example, if I'm in a row configuration for a table with one or more column splits, it could be helpful for a user to know that the ordering is being determined by the grand total of those columns. Here's an example shown in the design mockups:

image

Alternatives to the filter columns modal

Currently, when there is more than one split column dimension item, the user is presented with a modal when attempting to filter that column via the column popover menu. I understand the reason for doing so, as the concatenation of column headings into a single cell doesn't allow us to know exactly what dimension of the column the user wishes to filter by.

Regarding use of the modal component, I was thinking that it might be nice to keep the filtering experience contained within the column popover menu, if possible. For example, when the user clicks on filter action from the column popover menu, the options to pick which filters to apply replace the previous contents of the popover, keeping the experience contained in one location. Is that at all possible?

In any case (whether the above is possible or not), I suggest we change from the current usage of EuiSwitch to EuiCheckboxGroup, as toggling the switches doesn't perform an immediate action. Just let me know if design examples are needed.

Co-authored-by: Michael Marcialis <[email protected]>
@flash1293
Copy link
Contributor Author

@elasticmachine merge upstream

@flash1293
Copy link
Contributor Author

Indicate when ordering is by grand total

Let's discuss this in the issue I branched out for top values function changes. As this part of the UI is not specific to the data table, I'm hesitant with these changes because I'm not sure whether they still make sense in the rest of Lens - would people understand "grand total" ranking (we recently changed order to rank here) in an xy chart?

Regarding use of the modal component, I was thinking that it might be nice to keep the filtering experience contained within the column popover menu, if possible. For example, when the user clicks on filter action from the column popover menu, the options to pick which filters to apply replace the previous contents of the popover, keeping the experience contained in one location. Is that at all possible?

For a nice experience we would need to change the column actions popover from a list group to a full context menu, then we can have this kind of nested menu. However, it's difficult to achieve that in a fully integrated way because of the way filters work. It would be a bigger project involving the app arch team as well. The modal is a standard component which is used in a lot of places to apply multiple filters at once.

In any case (whether the above is possible or not), I suggest we change from the current usage of EuiSwitch to EuiCheckboxGroup, as toggling the switches doesn't perform an immediate action. Just let me know if design examples are needed.

That's a good point. As this piece of UI is not tied to the datatable or Lens, but used in all of Kibana, I will open a separate issue.

Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to approve this because it looks like it's totally functional and a good improvement. Most of my comments are about unexpected behavior that we can choose to address later.

Screen Shot 2021-03-10 at 6 21 49 PM

Screen Shot 2021-03-10 at 6 32 02 PM

return {
...originalDatatableColumn,
id: getTransposeId(uniqueValue, metricColumn.columnId),
name: `${uniqueValue} ${TRANSPOSE_VISUAL_SEPARATOR} ${originalDatatableColumn.name}`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found that the column naming here was not meeting my expectations in two different directions.

  1. For simple tables, I wanted to see a table with the unique value only. The metric name is not needed if there's only one metric, as long as the chart title conveys the meaning.

  2. I also wanted to see tables with the operation name, like Operation name > Unique value > Metric name. This would add context.

For an extreme example of this, try creating a single-row chart with multiple split columns. You'll see how the column labels get confusing:

Screen Shot 2021-03-10 at 6 49 20 PM

@flash1293
Copy link
Contributor Author

@elasticmachine merge upstream

@mbondyra mbondyra self-requested a review March 11, 2021 09:29
Copy link
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approved!

@flash1293
Copy link
Contributor Author

Thanks for the suggestions @wylieconlon - I think we can address those in a nice way once we introduce hierarchical column headers - it will make it much easier to put more and better information in there. I will open a separate issue to track this.

@flash1293 flash1293 merged commit 33fbe74 into elastic:master Mar 11, 2021
flash1293 added a commit to flash1293/kibana that referenced this pull request Mar 11, 2021
flash1293 added a commit that referenced this pull request Mar 11, 2021
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
lens 485 486 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
lens 921.8KB 935.3KB +13.5KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Lens release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.13.0 v8.0.0
Projects
None yet
10 participants