[Lens] New sorting feature for the datatable visualization #84435

dej611 · 2020-11-26T18:16:43Z

Summary

This PR implements the sorting feature for the datatable visualization in Lens.

Rather than using the Canvas sort function the lodash sorting implementation has been used in here: there's a compatibility issue between the MultiDatatable format and the Datatable accepted as input.

The feature reads the renderMode property to become interactive, and its stored in the state to be shown when in display mode.

Styling can probably be improved: a feature request has been submitted for a new readOnly state for Table sorting, in order to improve the View mode look and feel.

Checklist

Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)

dej611 · 2020-12-01T17:37:41Z

@elasticmachine merge upstream

elasticmachine · 2020-12-01T17:37:52Z

Pinging @elastic/kibana-app (Team:KibanaApp)

wylieconlon

Tested in Chrome and it's working consistently with Visualize. Accessibility looks good.

Can you think about how the title of this PR would appear in release notes, and whether we need to document this as part of the existing docs?

The "download as CSV" was not sorted- it makes sense, but is there something we can do to improve this?

wylieconlon · 2020-12-01T18:44:13Z

x-pack/plugins/lens/public/datatable_visualization/expression.tsx

+ let sortedRows = rows;
+
+ if (sortBy && sortDirection !== 'none') {
+ sortedRows = orderBy(rows, [sortBy], sortDirection as Direction);


It looks like we are sorting based on the unformatted value of the table cell, not the formatted value. I checked and this is the existing behavior of the Visualize app, but is this the best behavior? It does appear to be correct for numbers and dates using the standard formatters.

The buggiest case I could find is how we deal with IP address fields- the sort order looks totally wrong because it's sorting the unformatted value. What do you think the options are?

This is a good point. I'll improve on this side. 👍

@dej611 I would expect numbers and dates to be sorted by raw value and strings sorted by the formatted value (e.g. for stuff like static lookup formatters) .

What about other types? Using formatters for everything but numbers and dates?

wylieconlon · 2020-12-01T19:01:29Z

x-pack/plugins/lens/public/datatable_visualization/visualization.tsx

+ sorting?: {
+ columnId: string | undefined;
+ direction: 'asc' | 'desc';
+ };


Why is the sorting attached to the top level of the state, instead of attached to a specific layer?

Also, what happens in these two cases?

A sorted column is deleted- is the state correctly updated?

The columnId for a sorted column is reused but the function has changed. It's still sorted, but should it be?

Yes, I've thought about that and discussed a bit with @flash1293 about.
We agreed to have sorting on a visualization level for now, as tables can only have one layer.
When we'll extend tables support for multiple layer this feature will be revisited, as probably other changes are planned to happen to this code meanwhile.

No strong feeling about the sorting state - @dej611 and me discussed this but I think it's fine in both places. I wasn't aware we are supporting multiple layers in the first place, what's the reasoning for this? (it seems like it's not possible for the user to actually configure this and even if they did, it would just show the first table)

The columnId for a sorted column is reused but the function has changed. It's still sorted, but should it be?

IMHO it should still be sorted as long as it's the same dimension/column. If the dimension gets deleted completely, the sorting state should be deleted along with it.

A sorted column is deleted- is the state correctly updated?

Probably the state in this case will remain "dirty" but the final result should be correct anyway as it relies on the columnId. I'll try to address this scenario.

The columnId for a sorted column is reused but the function has changed. It's still sorted, but should it be?

~~I wasn't aware of recycled ids. I'll take care for that too.~~

I think it still makes sense to have the sorting on that column, even when the operation is changed.

Had a first look at this implementation: the formatters are available only at render time as dependency, not at expression time, going in conflict on the CSV sorted downloadable content.

I'll investigate on possible solutions. In case I'd rather prioritise this over the CSV sorted downloadable content.

Ah, good point. Agreed, we can open a separate issue for CSV sorted downloadable content. It's fixable by passing down the reference to the format factory from data plugin from the setup method of the DatatableVisualization service class in x-pack/plugins/lens/public/datatable_visualization/index.ts and wrapping the datatable visualization definition into a getter (getDatatableVisualizationDefinition(data: DataPublicPluginStart)). We are doing a similar thing in xy chart for passing down the palette service.

flash1293 · 2020-12-02T09:21:14Z

x-pack/plugins/lens/public/types.ts

@@ -608,6 +608,17 @@ export interface LensBrushEvent {
 data: TriggerContext<typeof SELECT_RANGE_TRIGGER>['data'];
 }

+export interface LensSortActionData {


Can you move this type into the datatable visualization and make LensEditEvent a generic type for all kinds of payloads?

flash1293 · 2020-12-02T09:22:42Z

The "download as CSV" was not sorted- it makes sense, but is there something we can do to improve this?

That's a good point - we can actually improve this by moving the sorting logic into the datatable expression function instead of the renderer and updating the tables adapter with the sorted table.

flash1293 · 2020-12-02T09:29:16Z

I didn't find a good way to "hide" the sorting functionality when the table is used on a dashboard - I'm mostly concerned about the sorting being explicitly called out via screenreader along with the visual indicators:

IMHO it's worth addressing this upstream before merging the PR.

…e-sorting

… into feature/lens/table-sorting

dej611 · 2020-12-03T10:49:48Z

Implemented a basic readOnly state for the sorting header.
The sorting header is yet not keyboard accessible: it is already an hacky solution, not sure if we want to keep going this direction. Even a small fix like elastic/eui#4333 may help on this side.

As an alternative I've also tried with a tooltip when in readOnly mode: it offers a cleaner solution, but then introduces inconsistencies with the edit mode. Unless we want to completely handle the sorting feature via custom tooltips

flash1293

I would be fine with this solution a11y-wise, but I would like to get @myasonik s expertise on this:

Michail, we want to show a sorted table and indicate that, but the user shouldn't be able to change the sorting. Unfortunately this is currently not supported by EuiBasicTable.

This PR implements a workaround by not telling the table it's sorting, but showing the icon and a screenreader text in the header cell manually.

Header cell during edit mode (default created by EUI component):

Header cell during view mode:

The only thing missing AFAICT is the aria-sort attribute on the th element (it's on the span within the th, but not sure whether it helps much there). The question is whether that's a big no-no or whether it's just not perfect. If it's a minor issue, we would like to get the feature into 7.11, otherwise we will take care of an upstream fix in EUI to do it right from the start.

You can test this by creating a Lens table and clicking the header to sort the table, then saving it to a dashboard.

flash1293 · 2020-12-03T13:13:30Z

x-pack/plugins/lens/public/types.ts

@@ -25,6 +25,7 @@ import {
 VALUE_CLICK_TRIGGER,
 VisualizeFieldContext,
 } from '../../../../src/plugins/ui_actions/public';
+import { LensSortActionData, LENS_EDIT_SORT_ACTION } from './datatable_visualization/expression';


This import is causing the current build to fail because all of the datatable vis is included in the initial bundle. This should be fixable by making it a type import: import type { LensSortActionData, LENS_EDIT_SORT_ACTION } from './datatable_visualization/expression';

flash1293 · 2020-12-03T13:16:23Z

x-pack/plugins/lens/public/datatable_visualization/expression.tsx

+ }
+ // This is a workaround to hijack the title value of the header cell
+ return (
+ <span aria-sort={getDirectionLongLabel(sorting.direction)} aria-live="polite">


This won't update, so no need to make it aria-live="polite"

flash1293 · 2020-12-03T13:19:00Z

x-pack/plugins/lens/public/datatable_visualization/expression.tsx

+ columnsReverseLookup[sortBy]?.meta?.type || ''
+ )
+ ? sortBy
+ : (row: Record<string, unknown>) => formatters[sortBy]?.convert(row[sortBy]);


This is not taking into account ip sorting, right? If Visualize datatable isn't doing it right either at the moment, I'm fine with resolving this later, but we should create an issue for it.

Update a first version of IPv4 sorting specific logic

myasonik · 2020-12-03T16:42:27Z

<sorting workaround for view-only tables>

IMO it's a minor issue. Screenreaders will read out the table sort better* when using aria-sort but this seems like a minor bug. If there's an issue for it in EUI to eventually get resolved, I'm not worried about this shipping.

As another workaround which may be uglier in the code but would be better for a11y: you could use a ref pointed to the table, query for the header cell, then headerCell.setAttribute('aria-sort', ${sortDirection}) to get the right attribute on the right element but I'm not sure how much overhead that might add.

*What does better mean?

Better just means assistive tech can/will read out the sort when entering the table from anywhere (jumping into it, going backwards into it, etc), whereas with the current solution a user wouldn't know the table is sorted without reading the column header.

flash1293 · 2020-12-03T17:10:29Z

As another workaround which may be uglier in the code but would be better for a11y: you could use a ref pointed to the table, query for the header cell, then headerCell.setAttribute('aria-sort', ${sortDirection}) to get the right attribute on the right element but I'm not sure how much overhead that might add.

This scares me, if we can live without it please let us do :)

@dej611 @wylieconlon I'd say let's try to get this merged before 7.11 as it seems very close to me. I can give it another look tomorrow.

flash1293

As discussed, the logic for changing the sort order should be moved inside of the datatable visualization, keeping the frame level code agnostic to specific events being fired.

Also, visualizationState should not re-create the handleEvent callback (this can be done by allowing an updater function to be used for visualization)

flash1293 · 2020-12-04T16:09:55Z

x-pack/plugins/lens/public/datatable_visualization/expression.tsx

+ if (type !== 'ip') {
+ return rowFormatted;
+ }
+ // Note this supports only IPv4: maybe there already a solution to support IPv6 and more?


Ip fields can also store ipv6 values in Elasticsearch: https://www.elastic.co/guide/en/elasticsearch/reference/current/ip.html

To me it feels weird to just support ip4, but always attempt to sort. IMHO we should split that feature out and disable sorting on ip columns for now and tackle that separately.

Yes, agree. There are also special values we may want to handle (localhost as/or 127.0.0.1?).

flash1293

This looks great to me!

The only thing I'm concerned about is the ip sorting, I left a comment about that.

Otherwise this LGTM.

Edit: Just discovered another edge case where it doesn't work correctly - ranges:

I think they should get sorted by the from, then by the to, but I'm not a fan of tying the datatable sorting algorithm to ranges specifically. I can't think of a better solution though - so for now I would be fine with checking whether a number column contains objects with a from or a to property and sort by them.

dej611 · 2020-12-04T18:35:52Z

I think they should get sorted by the from, then by the to, but I'm not a fan of tying the datatable sorting algorithm to ranges specifically. I can't think of a better solution though - so for now I would be fine with checking whether a number column contains objects with a from or a to property and sort by them.

Tried to implement this but it's not super trivial. I would rather keep the string comparison for the moment and probably build/get a tiny utility well tested for that.

I could still remove the IPv4 sorting logic if prefer to delay for a better solution.

flash1293 · 2020-12-04T20:28:59Z

I would go with the following:

Removing sorting support for ip completely (not even string based) - just don't make the column header clickable in that case. EuiBasicTable allows to configure this
Live with string sorting for ranges for now because we have no good way of handling the case, but create an issue to clean this up eventually

flash1293 · 2020-12-08T13:07:54Z

@elasticmachine merge upstream

flash1293 · 2020-12-14T07:51:52Z

@elasticmachine merge upstream

flash1293 · 2020-12-14T07:54:31Z

As discussed offline, for the first version we will just allow sorting ips and ranges and sort them as strings. Improving this will become a fix-it-week issue.

flash1293

Tested and everything works as expected, LGTM.

dej611 · 2020-12-14T18:30:09Z

#83167 changed some types in the EuiBasicTable component we had, generating some conflicts. Had to revisit few types to make it work with sorting as well.

wylieconlon

Tested the new behavior and LGTM, left some style comments

wylieconlon · 2020-12-14T20:49:53Z

x-pack/plugins/lens/public/editor_frame_service/editor_frame/state_management.ts

+ state:
+ typeof action.updater === 'function'
+ ? action.updater(state.visualization.state)
+ : action.updater,


This is a good change, but does it need to be in this PR specifically?

This PR was the place we realised about this possible change. Making it here helped fixing a useEffect side effect which was affecting the datatable visualization.

...ck/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx

…orkspace_panel/workspace_panel.tsx Co-authored-by: Wylie Conlon <[email protected]>

kibanamachine · 2020-12-15T11:04:54Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 6f65ab3

Metrics [docs]

Async chunks

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

id	before	after	diff
`lens`	1018.4KB	1023.4KB	+5.0KB

Distributable file count

id	before	after	diff
`default`	47130	47890	+760

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`lens`	53.7KB	54.1KB	+404.0B

History

💔 Build #94131 failed 8014026
💛 Build #93955 was flaky 121c701
💚 Build #93891 succeeded c531447
💚 Build #92687 succeeded fd1e9ba
💚 Build #92012 succeeded a5e1086

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

…4435) Co-authored-by: Wylie Conlon <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

) (#85987) Co-authored-by: Wylie Conlon <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

✨ New sorting feature for the datatable visualization

2010f76

dej611 added release_note:enhancement v8.0.0 Feature:Lens v7.11.0 labels Nov 26, 2020

dej611 added 2 commits November 30, 2020 12:08

✅ Add some unit testing for table storting

9534f6a

✅ Add test for column switch

f92107c

dej611 marked this pull request as ready for review December 1, 2020 17:37

dej611 requested a review from a team December 1, 2020 17:37

Merge branch 'master' into feature/lens/table-sorting

d786a30

dej611 added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Dec 1, 2020

wylieconlon reviewed Dec 1, 2020

View reviewed changes

flash1293 reviewed Dec 2, 2020

View reviewed changes

dej611 added 5 commits December 2, 2020 16:01

🐛 Fix the dimension removal scenario

11135c5

🏷️ Refactor types for edit events

3877af0

💄 Implement readOnly state for sorting

cb5006c

Merge remote-tracking branch 'upstream/master' into feature/lens/tabl…

18e925c

…e-sorting

Merge branch 'feature/lens/table-sorting' of github.com:dej611/kibana…

ea96b1a

… into feature/lens/table-sorting

✅ Fix type issue + add test for readOnly mode

81396a4

flash1293 reviewed Dec 3, 2020

View reviewed changes

dej611 added 2 commits December 3, 2020 15:26

🐛 Fix bundle size issue

468005d

✨ Make sorting happen at the expression level

59a2715

flash1293 reviewed Dec 4, 2020

View reviewed changes

♻️ Refactor edit event flow + vis state updater action

a5e1086

flash1293 reviewed Dec 4, 2020

View reviewed changes

Merge branch 'master' into feature/lens/table-sorting

fd1e9ba

dej611 mentioned this pull request Dec 9, 2020

[Lens] Provide a Parser and Comparable util for IP addresses #85431

Closed

Merge branch 'master' into feature/lens/table-sorting

c531447

flash1293 approved these changes Dec 14, 2020

View reviewed changes

dej611 added 2 commits December 14, 2020 12:46

🔥 Remove IPv4 specific sorting logic for now

121c701

🔀 Merge latest master and fix conflicts

8014026

wylieconlon approved these changes Dec 14, 2020

View reviewed changes

dej611 and others added 3 commits December 15, 2020 09:46

Update x-pack/plugins/lens/public/editor_frame_service/editor_frame/w…

406dd81

…orkspace_panel/workspace_panel.tsx Co-authored-by: Wylie Conlon <[email protected]>

📸 Update snapshot

a421bbb

🐛 Fix suggestion conflict syntax issue

6f65ab3

dej611 merged commit 62623cd into elastic:master Dec 15, 2020

dej611 deleted the feature/lens/table-sorting branch December 15, 2020 11:38

dej611 mentioned this pull request Dec 15, 2020

[7.x] [Lens] New sorting feature for the datatable visualization (#84435) #85987

Merged

dej611 added a commit to dej611/kibana that referenced this pull request Dec 15, 2020

[Lens] New sorting feature for the datatable visualization (elastic#8…

821e9f9

…4435) Co-authored-by: Wylie Conlon <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

dej611 added a commit that referenced this pull request Dec 16, 2020

[7.x] [Lens] New sorting feature for the datatable visualization (#84435

91dacb6

) (#85987) Co-authored-by: Wylie Conlon <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

dej611 mentioned this pull request Dec 30, 2020

[Lens] Add specific IP and Range/Interval sorting to datatable #87006

Merged

1 task

[Lens] New sorting feature for the datatable visualization #84435

[Lens] New sorting feature for the datatable visualization #84435

Conversation

dej611 commented Nov 26, 2020 • edited Loading

Summary

Checklist

dej611 commented Dec 1, 2020

elasticmachine commented Dec 1, 2020

wylieconlon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dej611 Dec 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash1293 Dec 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash1293 commented Dec 2, 2020

flash1293 commented Dec 2, 2020

dej611 commented Dec 3, 2020

flash1293 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myasonik commented Dec 3, 2020

flash1293 commented Dec 3, 2020

flash1293 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash1293 left a comment • edited Loading

Choose a reason for hiding this comment

dej611 commented Dec 4, 2020 • edited Loading

flash1293 commented Dec 4, 2020

flash1293 commented Dec 8, 2020

flash1293 commented Dec 14, 2020

flash1293 commented Dec 14, 2020

flash1293 left a comment

Choose a reason for hiding this comment

dej611 commented Dec 14, 2020

wylieconlon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Dec 15, 2020

💚 Build Succeeded

Metrics [docs]

Async chunks

Distributable file count

Page load bundle

History

dej611 commented Nov 26, 2020 •

edited

Loading

dej611 Dec 2, 2020 •

edited

Loading

flash1293 Dec 2, 2020 •

edited

Loading

flash1293 left a comment •

edited

Loading

flash1293 left a comment •

edited

Loading

dej611 commented Dec 4, 2020 •

edited

Loading