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] ignore global query layer setting #35542

Merged
merged 17 commits into from
Apr 27, 2019

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Apr 24, 2019

implements #33041

Screen Shot 2019-04-25 at 11 45 53 AM

When layer does not support filtering like Tile layer or Vector layer from EMS without a join, the checkbox is disabled

Screen Shot 2019-04-25 at 11 45 46 AM

This PR adds applyGlobalQuery setting to layer. When false, source data fetches, join data fetches and get bounds data fetches will ignore global query bar and filter pills.

Index patterns that are only applied to layers that disable applyGlobalQuery will not provide type ahead support in query bar or be selectable in filter pill editor

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@nreese nreese added [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation release_note:enhancement labels Apr 24, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis

@elasticmachine
Copy link
Contributor

💔 Build Failed

@nreese
Copy link
Contributor Author

nreese commented Apr 24, 2019

@cchaos @thomasneirynck Should Filter panel always be displayed for all vector layers?

Currently, the Filter panel is only displayed for vector layers with es_search_source or es_grid_source. Adding applyGlobalQuery changes things since that setting is in the Filter panel and applies to terms joins. So EMS vector layers need the Filter panel when the layer has terms joins and does not need the section when there are no terms joins.

For example: if you create an EMS vector layer of the world countries the details panel looks like this
Screen Shot 2019-04-24 at 3 20 20 PM

But, once a join is added, it looks like this
Screen Shot 2019-04-24 at 3 21 03 PM

Is it weird to have the Filter panel appear and disappear? Would it be better to always display it and just disable applyGlobalQuery switch until a terms join is configured?

Just for context, here is what the layer details panel looks like with a vector layer from es_search_source. The Filter section is always displayed since the layer will always pull data from elasticsearch
Screen Shot 2019-04-24 at 3 24 53 PM

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@cchaos
Copy link
Contributor

cchaos commented Apr 25, 2019

I was having an issue understanding what this toggle does and had to go through the entire issue and PR thread to understand what the toggle does. It was almost more clear in TSVB when labelled as "Ignore global filters". Though it is weird to have a toggle with a negative label. Maybe a word other than "Apply" which is more of an action item vs a passive "Let it be" type of thing.

I'm also not entirely following why the "Apply global filters" is tied to joins, and based on the screenshots, I think it will not be clear to the user either. If filters are directly affected by joins then the filters panel should come after the joins panel.

Though, it seems more of an overall layer setting that should be able to be switched on/off no matter the layer type. Why can't you just always say "Never apply global filters" not matter what other settings they have/don't have selected for the layer? That way the option is always available, but may not do anything now because it's not actually tied to an index pattern yet. You can then keep it in one place for all layers and not have it inside the Filter panel.

All that said, I'm always a proponent of disabling rather than hiding and then adding tooltips explaining why something is disabled.

@nreese
Copy link
Contributor Author

nreese commented Apr 25, 2019

Though, it seems more of an overall layer setting that should be able to be switched on/off no matter the layer type. Why can't you just always say "Never apply global filters" not matter what other settings they have/don't have selected for the layer?

I think always showing an active Apply global query button for all layers would be confusing. This would imply that all layers are filtered by the global query bar. That is not true. Only layers coming from Elasticsearch are filtered by the global query bar.

You can then keep it in one place for all layers and not have it inside the Filter panel.

Should i move the Apply global query switch to layer settings panel?

All that said, I'm always a proponent of disabling rather than hiding and then adding tooltips explaining why something is disabled.

I like the idea of always showing but having the Apply global query switch being disabled for layers that do not fetch data from Elasticsearch. That way it would be easy to see when layers are and are not fetching data from Elasticsearch and explain that queries do/do not effect the layer.

@cchaos
Copy link
Contributor

cchaos commented Apr 25, 2019

Should i move the Apply global query switch to layer settings panel?

Makes the most sense to me.

@alexfrancoeur
Copy link

@cchaos I agree, I had some trouble with Apply global query as well. But I wasn't sure if I was just too used to the way TSVB does it. @gchaps is there any chance you might be able to provide a suggestion? We're looking to provide a switch that will either enable or disable filters from the query bar / dashboard to be applied to that particular layer.

For instance, if I wanted to filter on a specific flights the airports but they were two different indices if global filters were applied, the airport layer would disappear. If global filters were ignored, they would persist even though the flights layer is being filtered.

@nreese
Copy link
Contributor Author

nreese commented Apr 25, 2019

Thanks for feedback. I have moved the checkbox to Layer settings panel.

Screen Shot 2019-04-25 at 11 45 53 AM

The checkbox is disabled if the layer does not support filtering

Screen Shot 2019-04-25 at 11 45 46 AM

@nreese
Copy link
Contributor Author

nreese commented Apr 25, 2019

@alexfrancoeur @cchaos @thomasneirynck Should the Filter panel be removed and then the filter input be moved into the Source settings panel? This would make it clearer that the filter only applies to the source and not joins.

@nreese nreese added the review label Apr 25, 2019
@cchaos
Copy link
Contributor

cchaos commented Apr 25, 2019

Filters is too much it's own beast and can be quite large. I'd leave it as it's own.

Also based on your screenshot above, if the toggle can't be used and is disabled it should also be in the "off" position.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@nreese
Copy link
Contributor Author

nreese commented Apr 25, 2019

Just chatted with @gchaps and we decided the best label was Apply global filter to layer and the tooltip for disabled checkbox would say Layer does not support filtering.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@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.

tested and works. neat!

a little unrelated, but isn't the layer-settings panel getting a little unbalanced? (cc @cchaos)

image

e.g. should we remove Display layer when map is in zoom range. it's a little fluffy

and the global-query switch doesn't have a little heading, while all other 3 settings do.

@cchaos
Copy link
Contributor

cchaos commented Apr 26, 2019

should we remove Display layer when map is in zoom range. it's a little fluffy

The label for that element didn't give a good understanding of the control which is why the help text was necessary. However the spacing seems really off and there might be a way to change the label text to just be more verbose like Map zoom range for layer visibility or something.

the global-query switch doesn't have a little heading, while all other 3 settings do

That's how labels are handled for switches vs inputs. We know there's a bit of a disparity and may alter that later in EUI, but I'd leave it as is.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@gchaps
Copy link
Contributor

gchaps commented Apr 26, 2019

Going with @cchaos suggestion, how about this for the label

Zoom range for layer visibility

and then removing the fluffy text.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@nreese
Copy link
Contributor Author

nreese commented Apr 27, 2019

jenkins, test this

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@nreese nreese merged commit 8c687ed into elastic:master Apr 27, 2019
nreese added a commit to nreese/kibana that referenced this pull request Apr 27, 2019
* [Maps] ignore global query layer setting

* do not include index pattern id in query bar indexPatterns when applyGlobalQuery is disabled

* update how embeddable factory extracts indexPatterns so only queryable index patterns are included in list

* support applyGlobalQuery for heatmap and getBounds

* add functional tests to join layer

* show filter section when layer has join

* move checkbox to layer settings panel

* text review

* set checkbox to false when disabled

* do not trigger refetch when global query and global filter changes and applyGlobalQuery is disabled

* rename applyGlobalQuery to getApplyGlobalQuery

* throw error in map embeddable factory if layerListJSON can not be parsed

* remove extra space

* update zoom range slider label and remove nested EuiFormRow
nreese added a commit that referenced this pull request Apr 28, 2019
* [Maps] ignore global query layer setting

* do not include index pattern id in query bar indexPatterns when applyGlobalQuery is disabled

* update how embeddable factory extracts indexPatterns so only queryable index patterns are included in list

* support applyGlobalQuery for heatmap and getBounds

* add functional tests to join layer

* show filter section when layer has join

* move checkbox to layer settings panel

* text review

* set checkbox to false when disabled

* do not trigger refetch when global query and global filter changes and applyGlobalQuery is disabled

* rename applyGlobalQuery to getApplyGlobalQuery

* throw error in map embeddable factory if layerListJSON can not be parsed

* remove extra space

* update zoom range slider label and remove nested EuiFormRow
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants