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] fix choropleth map with applyGlobalQuery set to false still creates filter for source. #108999

Merged
merged 11 commits into from
Aug 24, 2021

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Aug 17, 2021

Fixes #64861.

When creating a phrase filter for a layer with joins, a filter should not be created for the left source when the left source is a Documents source and applyGlobalQuery is set to false. Using map.regionmap.* has been a work around for this issue. This workaround will being going away in 8.0 with removal of map.regionmap.* kibana.yml settings.

@nreese nreese added release_note:fix [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v8.0.0 auto-backport Deprecated - use backport:version if exact versions are needed v7.16.0 labels Aug 17, 2021
@nreese nreese requested a review from a team as a code owner August 17, 2021 20:50
@elasticmachine
Copy link
Contributor

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

@@ -75,7 +75,6 @@ export interface IVectorSource extends ISource {
defaultFields: Record<string, Record<string, string>>
): Promise<void>;
deleteFeature(featureId: string): Promise<void>;
isFilterByMapBounds(): boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not relevant to this fix, but this method exists ISource and does not need to be duplicated in VectorSource

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.

Really improves the default-behavior.

discussed offline; since this is now more "automagic", it might not be clear to an end-user why a different filter is created than what the name in the tooltip would suggest.

image

^ perhaps we can make the iso2-row standout as a special "join-row" (e.g. using chain icon), with some helptext in a tooltip explaining its purpose.

@nreese
Copy link
Contributor Author

nreese commented Aug 19, 2021

@elasticmachine merge upstream

@nreese
Copy link
Contributor Author

nreese commented Aug 19, 2021

^ perhaps we can make the iso2-row standout as a special "join-row" (e.g. using chain icon), with some helptext in a tooltip explaining its purpose.

Added a "link" icon with tooltip to show that a field is a join field.

Screen Shot 2021-08-19 at 4 13 18 PM

@gchaps Maybe you can review at the copy? Not sure if its too long and if the first sentence is even needed.

@gchaps
Copy link
Contributor

gchaps commented Aug 20, 2021

I agree that the tooltip is a little long. Here's the text we brainstormed:

Shared key 'ISO 3166-1 alpha' is joined with 'geo.src'.

@nreese
Copy link
Contributor Author

nreese commented Aug 20, 2021

@elasticmachine merge upstream

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.

thx for the clean bolt-on. it's a nice fix, and one less roadblock for removing the regionmap.* config. i also like the new tooltip-row. It fits the use-case imho.

@nreese
Copy link
Contributor Author

nreese commented Aug 24, 2021

@elasticmachine merge upstream

@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
maps 3.2MB 3.2MB +930.0B
Unknown metric groups

References to deprecated APIs

id before after diff
maps 685 688 +3

History

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

@nreese nreese merged commit 6963c0a into elastic:master Aug 24, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Aug 24, 2021
…eates filter for source. (elastic#108999)

* [Maps] fix choropleth map with applyGlobalQuery set to false still creates filter for source.

* cleanup functional test

* eslint

* fix functional test

* add inidication in tooltip when field is join key

* copy updates

* update jest test

* eslint

Co-authored-by: Kibana Machine <[email protected]>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Aug 24, 2021
…eates filter for source. (#108999) (#109915)

* [Maps] fix choropleth map with applyGlobalQuery set to false still creates filter for source.

* cleanup functional test

* eslint

* fix functional test

* add inidication in tooltip when field is join key

* copy updates

* update jest test

* eslint

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Nathan Reese <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation release_note:fix v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Maps] choropleth map with applyGlobalQuery set to false still creates filter for source.
5 participants