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] add draw filter action to layer #33686

Merged
merged 35 commits into from
May 7, 2019

Conversation

thomasneirynck
Copy link
Contributor

@thomasneirynck thomasneirynck commented Mar 22, 2019

Closes #33658

Companion to #33635, addresses spatial filtering item of #31178.


This now introduces a toolbar

Toolbar contains draw controls

image

Selecting draw type requires index and field selection

image

Draw shape and filter

image

@thomasneirynck thomasneirynck added discuss WIP Work in progress [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v8.0.0 v7.2.0 labels Mar 22, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis

@thomasneirynck thomasneirynck requested review from nreese and kindsun and removed request for nreese March 22, 2019 04:13
@elasticmachine
Copy link
Contributor

💔 Build Failed

@alexfrancoeur
Copy link

Is the plan to add a global one as well? I'd imagine if users are filtering, they'd like to across all layers when on a dashboard for instance.

@thomasneirynck
Copy link
Contributor Author

thomasneirynck commented Mar 22, 2019

Is the plan to add a global one as well?

We can. I'm just not sure how this would behave. If we add a global filter, we'll need to create the same shape filter for each and every index-pattern on the map. I'm concerned the filter bar will look really cluttered. Filter-management would get complicated.

It's just not an issue we're running into with coordinate-maps (since it's only a single layer), but we are running into here. Not sure what a good flow would be.

@nreese
Copy link
Contributor

nreese commented Mar 24, 2019

After the filter is created, the editable shape disappears from the map

I think it would be nice to have a way of showing the geospatial filters that are applied to the map so users can see where their filters are

@thomasneirynck thomasneirynck requested a review from a team as a code owner March 27, 2019 17:10
@elasticmachine
Copy link
Contributor

💔 Build Failed

@thomasneirynck
Copy link
Contributor Author

I think it would be nice to have a way of showing the geospatial filters that are applied to the map so users can see where their filters are

I agree this is useful, but it adds a whole new layer of functionality to the app. I guess we would end up adding something like a "filter layer" (?). Would this be present in the table of contents as a top-level layer (?), with all the associated actions/features?

Perhaps we could tackle that concept more as part of an "annotation" layer. It would be a new layer-type, not backed by a source. But people could draw any kind of shape on the map, in a dedicated layer. Then, we could associate actions with these shapes (e.g. create filter for this shape, query nearby features, ....).

Or do you see lower hanging fruit?

@elasticmachine
Copy link
Contributor

💔 Build Failed

@alexfrancoeur
Copy link

@thomasneirynck new PR or issue? I was hoping to test out today haha

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@thomasneirynck
Copy link
Contributor Author


Below is mostly obsolete (historical purposes)

POC of how we may might possibly do shape filtering on the map.

The biggest issue is that this needs to function across multiple index-patterns.

This PR proposes that spatial filters are layer-based and part of the layer-actions. After drawing the shape, a filter is added to the filter bar, which will apply to all visualizations that are backed by that index pattern.

For layers which are not backed by an es index-pattern that is spatially filterable the shape filtering is disabled.

After the filter is created, the editable shape disappears from the map.

This is all very close to how spatial filtering now works on coordinate maps. The biggest difference is that with multiple layers, multiple spatial filters could be created for multiple index patterns.

image


UPDATE

Trying a differnent approach:

Have top level filter tool buttons. On opening, they present the user with a list of indexpattern-geofields they can use to create the filter.

Added a tool for both shape and bounds drawing.

wrt. the bounds drawing tool. This relies on a 3d party dependency thrice removed (https://github.com/mapbox/mapbox-gl-draw). It is a plugin, on to of a mapbox-drawing plugin, which in turn is a plugin for the mapbox-gl library. If rectangle bounds filtering is not strictly necessary, I would prefer not to see this included in a first pass at this funtionality. (I also don't think we should get into the business of writing our own drawing tools, at least at this stage, given the limited added-value it would add wrt. required work/maintenance).

image

@thomasneirynck thomasneirynck removed the WIP Work in progress label Apr 22, 2019
@thomasneirynck thomasneirynck requested a review from nreese May 6, 2019 21:47
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

}

componentDidUpdate() {
this._getuniqueIndexPatternAndFieldCombos();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does _getuniqueIndexPatternAndFieldCombos need to get called on every state/prop change. How about just calling _getuniqueIndexPatternAndFieldCombos inside componentDidMount and inside _toggleToolbar when opening the popover?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Visibility of the tool-button depends on whether there are filterable-layers to begin with, it also needs to be called at every update of props.uniqueIndexPatternIds (as layers are added).

afaic, I would just leave it as-is, instead of trying to optimize this away.

@thomasneirynck thomasneirynck requested a review from nreese May 7, 2019 16:20
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@thomasneirynck thomasneirynck requested a review from cchaos May 7, 2019 17:34
@thomasneirynck thomasneirynck requested a review from nreese May 7, 2019 19:15
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Re-tested and works for me.

@nreese nreese mentioned this pull request May 7, 2019
10 tasks
Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

lgtm with green CI
code review, tested in chrome

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@tylersmalley
Copy link
Contributor

This breaks IE11, so we might want to revert until we can resolve. There are a few issues on the repo regarding this issue:

mapbox/mapbox-gl-draw#638
mapbox/mapbox-gl-draw#823

@tylersmalley
Copy link
Contributor

I have opened a fix here: #36505

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] Filter dashboards by drawing on map
6 participants