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

[Maps] Move sort out of top hits configuration for ES documents source #47361

Merged
merged 11 commits into from
Oct 9, 2019

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Oct 4, 2019

closes #46882 and #47279

This PR replaces topHitsTimeField with sortField and sortOrder and allows users to sort documents and top hit documents by any sortable field, including numbers

Screen Shot 2019-10-04 at 11 28 54 AM

@nreese nreese added release_note:enhancement [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v8.0.0 v7.5.0 labels Oct 4, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis (Team:Geo)

@elasticmachine
Copy link
Contributor

💔 Build Failed

@nreese nreese requested a review from nickpeihl October 4, 2019 18:43
@nreese
Copy link
Contributor Author

nreese commented Oct 4, 2019

cc @Alex3k This PR allows you to sort top hits by numerical fields if you want to try it out and ensure this solves your use case

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@nreese
Copy link
Contributor Author

nreese commented Oct 7, 2019

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@nreese
Copy link
Contributor Author

nreese commented Oct 7, 2019

another failure in machine_learning/anomaly_detection/saved_search_job·ts

retest

Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

mostly nits tbh. Agreed that this is a nice improvement, and might enable a few more use-cases.

disabled={!this.props.sortField}
options={[
{ text: 'ASC', value: SORT_ORDER.ASC },
{ text: 'DESC', value: SORT_ORDER.DESC }
Copy link
Contributor

Choose a reason for hiding this comment

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

ascending/descending fully spelled out? (and localize i18n too)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am hesitant to do this since spelling out ascending/descending will take a lot of horizontal space and shorten the sort field select. IMHO, the sort field select needs to be as long as possible to handle long field names. Spelling out asc/desc adds little value compared to more room for long field names

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I could change the select to just an up or down icon?

Copy link
Member

Choose a reason for hiding this comment

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

A fully spelled out "Ascending" and "Descending" select box appears to be consistent with other visualizations.

@@ -56,9 +56,10 @@ export class ESSearchSource extends AbstractESSource {
geoField: descriptor.geoField,
filterByMapBounds: _.get(descriptor, 'filterByMapBounds', DEFAULT_FILTER_BY_MAP_BOUNDS),
tooltipProperties: _.get(descriptor, 'tooltipProperties', []),
sortField: _.get(descriptor, 'sortField', ''),
sortOrder: _.get(descriptor, 'sortOrder', SORT_ORDER.DESC),
Copy link
Contributor

Choose a reason for hiding this comment

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

consider wrapping this in single config object

sort: {
 order: ...,
 field: ...
}

looks a little cleaner, as they are co-dependent

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

lgtm, fwiw, I have no opinion either way on the ascending/descending. Great addition.

@nreese nreese merged commit c17081a into elastic:master Oct 9, 2019
nreese added a commit to nreese/kibana that referenced this pull request Oct 9, 2019
elastic#47361)

* [Maps] Move sort out of top hits configuration for ES documents source

* add migration script to convert topHitsTimeField to sortField

* update i18n translations

* add jest test for es docs source UpdateSourceEditor component

* remove time configuration from top hits docs

* update migrations integration expect statement

* review feedback

* reverse hits list so top documents by sort are drawn on top

* update functional test expect to account for reversing hits order

* update another functional test expect clause for reversing hits
nreese added a commit that referenced this pull request Oct 9, 2019
#47361) (#47732)

* [Maps] Move sort out of top hits configuration for ES documents source

* add migration script to convert topHitsTimeField to sortField

* update i18n translations

* add jest test for es docs source UpdateSourceEditor component

* remove time configuration from top hits docs

* update migrations integration expect statement

* review feedback

* reverse hits list so top documents by sort are drawn on top

* update functional test expect to account for reversing hits order

* update another functional test expect clause for reversing hits
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Maps] Support most recent document by entity to be numeric field
4 participants