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

[Discover] Change default sort handling #85561

Merged

Conversation

kertal
Copy link
Member

@kertal kertal commented Dec 10, 2020

Summary

This PR changes the way default sorting is handled in Discover. It was not possible to remove sorting. With the introduction of EuiDataGrid in #67259 this will mandatory, the user can't reset sorting with the legacy behavior (nothing happens, no explanation why). Also it makes sense to allow the user the get a unsorted view.

Before:
When there's no sorting given in state, the index patterns default sorting is used (index pattern with time field: by indexPattern.timeFieldName, index pattern without time field: by _score) when sending the request to ES.

After:
With this PR, the default sorting is pre-selected in state, so it's removable. In case sort in state is empty, sorting by _doc is used for index pattern with timefield, and _score for index pattern without timefield.

Testing
There is no way to unsort in UI until #67259, but you could edit the saved search saved object, setting sort to '[]'. Checking if it's sorted correctly in Discover and the saved search embeddable.

Checklist

@kertal kertal self-assigned this Dec 10, 2020
@@ -32,6 +31,6 @@ export function getDefaultSort(
if (indexPattern.timeFieldName && isSortable(indexPattern.timeFieldName, indexPattern)) {
return [[indexPattern.timeFieldName, defaultSortOrder]];
} else {
return [['_score', defaultSortOrder]];
Copy link
Member Author

Choose a reason for hiding this comment

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

quick question @timroes , I don't think it makes much sense to sort by _score by default when it's an index pattern without timefield. this function now returna an empty default sorting, which would mean, that ES get's the info sort by _doc. I think that's a better choice here, would you agree?

Copy link
Member Author

Choose a reason for hiding this comment

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

@timroes update: changed the default to _score for index patterns without time field (so it is handled like before and nothing breaks)

@kertal kertal added the Feature:Discover Discover Application label Dec 10, 2020
@kertal kertal marked this pull request as ready for review December 14, 2020 08:37
@kertal kertal requested a review from a team December 14, 2020 08:37
@kertal kertal added release_note:enhancement v7.11.0 v8.0.0 Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Dec 14, 2020
@elasticmachine
Copy link
Contributor

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

@kertal
Copy link
Member Author

kertal commented Dec 14, 2020

@elasticmachine merge upstream

@kertal kertal requested a review from timroes December 14, 2020 14:17
@kertal
Copy link
Member Author

kertal commented Dec 14, 2020

@elasticmachine merge upstream

@kertal kertal added v7.12.0 and removed v7.11.0 labels Dec 15, 2020
@kertal kertal requested a review from majagrubic December 15, 2020 16:15
@@ -26,12 +25,12 @@ import { SortOrder } from '../components/table_header/helpers';
* the default sort is returned depending of the index pattern
*/
export function getDefaultSort(
indexPattern: IndexPattern,
indexPattern: IndexPattern | undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

In what scenario would index pattern be undefined?

Copy link
Member Author

@kertal kertal Dec 18, 2020

Choose a reason for hiding this comment

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

It shouldn't. But when using this function in an embeddable, according the TypeScript it could be. So the decision was the edit in the function or in the embeddable. decided to modify here, since the function would work without an index pattern.

Copy link
Contributor

@majagrubic majagrubic left a comment

Choose a reason for hiding this comment

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

Tested according to instructions and works well. Just one minor question above and I think this is good to go 🎉

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

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
discover 451.4KB 452.2KB +769.0B

Distributable file count

id before after diff
default 47298 48058 +760

History

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

@kertal kertal merged commit 4bf2de7 into elastic:master Dec 18, 2020
kertal added a commit to kertal/kibana that referenced this pull request Dec 18, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Dec 21, 2020
* master: (48 commits)
  Fix request with disabled aggregation (elastic#85696)
  [Security Solution][Detections][Threshold Rules] Threshold Rule Bug Fixes (elastic#84918)
  Removed a possibility to define two different names for Alert types on API and UI level. (elastic#86236)
  Bump Node.js from version 14.15.2 to 14.15.3 (elastic#86593)
  [index patterns] Fleep app - Keep saved object field list until field caps provides fields (elastic#85370)
  [Security Solutions] fix timeline tabs + layout (elastic#86581)
  Upgrade to hapi version 20 (elastic#85406)
  App Services: Remove remaining uiActions, expressions, data, embeddable circular dependencies. (elastic#82791)
  Rename chartLibrary setting to legacyChartsLibrary (elastic#86529)
  [CI] TeamCity updates (elastic#85843)
  [Maps] Use Json for mvt-tests (elastic#86492)
  [Rollup Jobs] Added autofocus to cron editor (elastic#86324)
  [Monitoring][Alerting] CCR read exceptions alert (elastic#85908)
  [CI] Bump memory for main CI workers (elastic#86541)
  Explicitly set Elasticsearch heap size during CI and local development (elastic#86513)
  [App Search] Updates to results on the documents view (elastic#86181)
  [Discover] Change default sort handling  (elastic#85561)
  [App Search] Convert DocumentCreationModal to DocumentCreationFlyout (elastic#86508)
  [App Search] Sample Engines should have access to the Crawler (elastic#86502)
  Fixed duplication of create new modal (elastic#86489)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Discover Discover Application release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants