-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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 for index patterns without timefield #54427
[Discover] Change default sort handling for index patterns without timefield #54427
Conversation
- when there's no default search in state (for index pattern without timefield name). The default sort is set only before the query is sent to ES)
…-10-discover-fix-sort
Pinging @elastic/kibana-app (Team:KibanaApp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I see a couple disadvantages to taking _score
out of the app state:
- If the user adds the
_score
field as a column in the doc table, the UI won't show them that_score
is being sorted on. _score
and timestamp are both defaults, but they work differently now. If I'm on a non-timebased index pattern and I manually sort on a field, then I switch to a timebased index pattern, I get a multi sort with the first field plus the time field.
Switching index patterns should probably never modify a sort the the user has explicitly added themselves. And I think the UI should always be able to reflect what field is being sorted on even if it is a default.
good points, thx, I will think how to further improve this solution! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this line can be removed since getSort.array no longer takes a defaultOrder
https://github.com/elastic/kibana/blob/master/src/legacy/core_plugins/kibana/public/discover/np_ready/angular/discover.js#L523
Otherwise looks good! Nice work 👍
@elasticmachine merge upstream |
…-10-discover-fix-sort
…ertal/kibana into kertal-pr-2020-01-10-discover-fix-sort
Thx @Bargs , i've removed the line. |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
…mefield (elastic#54427) Default sort is no longer in state. There's now a separate function to provide default sort for ES and UI, in case the user didn't actively select a field to sort by
Summary
Before this PR, when using index patterns without a timefield, the sorting by
_score
was added to the state if there was no sort set by user / url. Before multi sort was implemented, this sorting was replaced when switching to an indexpattern with timefield, or when the user adds a sort by field. With the new multisort, switching to another index pattern or selecting another sort field, adds the new field in the sort array in state ( so sorting by_score
becomes sorting by_score
,newTimefield
).Solution
The default sort is only used in UI and before sending the Request to ES. It's not added to state. This is only done after user interaction.
You can test this by using our demo data, adding a new index pattern
kibana_sample*
without a timefield.Fixes #49050
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.- [ ] This was checked for cross-browser compatibility, including a check against IE11- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support- [ ] Documentation was added for features that require explanation or tutorials- [ ] Unit or functional tests were updated or added to match the most common scenarios- [ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers
- [ ] This was checked for breaking API changes and was labeled appropriately- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately