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] Show spatial filters on map to provide context when for active filters #63406

Merged
merged 25 commits into from
Apr 22, 2020

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Apr 13, 2020

fixes #14821

With this PR, the map will show the spatial filter so users have some reference of where their filters are getting applied.

Screen Shot 2020-04-13 at 2 30 56 PM

This PR is just a MVP and there are a lot of features that could be added in the future, including:

  1. ability to edit shape
  2. ability to customize style spatial filter layer. This will require map properties
  3. ability to turn functionality off. This will require map properties

@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.8.0 labels Apr 13, 2020
@nreese nreese requested review from jsanz and thomasneirynck April 13, 2020 20:36
@nreese nreese requested a review from a team as a code owner April 13, 2020 20:36
@elasticmachine
Copy link
Contributor

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

@nreese nreese mentioned this pull request Apr 13, 2020
Copy link
Member

@jsanz jsanz left a comment

Choose a reason for hiding this comment

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

Nice UI touch 👏

If anything, for readability, I would add a space after the comma, and four decimals should be enough since that's around 10 meters precision, even we may want to leave that as a future config parameter for the map since this may make sense to tweak for very specific city-level mapping.

Going further (maybe too much), could the number of decimals be derived from the distance? For a 600km filter having no decimals is ok, for a 500m filter having 4 decimals would not be enough. Apart from the readability criteria, that horizontal space for filter pills feels expensive, so some logic to use it well makes sense to me. What do you think?

Tested locally on Chromium

image

@nreese nreese changed the title Spatial filters layer [Maps] Show spatial filters on map to provide context when for active filters Apr 14, 2020
@@ -250,40 +254,4 @@ describe('mb/utils', () => {
const nextStyle = getMockStyle(nextLayerListOrder);
expect(orderedStyle).toEqual(nextStyle);
});

test('should reorder foo and bar and remove baz', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed these tests because they do not test anything new.

@nreese
Copy link
Contributor Author

nreese commented Apr 14, 2020

If anything, for readability, I would add a space after the comma, and four decimals should be enough since that's around 10 meters precision, even we may want to leave that as a future config parameter for the map since this may make sense to tweak for very specific city-level mapping.
Going further (maybe too much), could the number of decimals be derived from the distance? For a 600km filter having no decimals is ok, for a 500m filter having 4 decimals would not be enough. Apart from the readability criteria, that horizontal space for filter pills feels expensive, so some logic to use it well makes sense to me. What do you think?

Good idea. I have updated the logic for the distance filter generation to lower precision as the distance increases and the precision is not as useful

@nreese nreese requested a review from jsanz April 14, 2020 16:48
Copy link
Member

@jsanz jsanz left a comment

Choose a reason for hiding this comment

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

I have updated the logic for the distance filter generation to lower precision as the distance increases and the precision is not as useful

Nice!! 👏 👏

image

From the functional point of view, I have nothing else to comment on, great new feature.

@nreese
Copy link
Contributor Author

nreese commented Apr 16, 2020

@elasticmachine merge upstream

@nreese
Copy link
Contributor Author

nreese commented Apr 16, 2020

@elasticmachine merge upstream

@nreese
Copy link
Contributor Author

nreese commented Apr 20, 2020

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

I like it, but I'm wondering if we should introduce this point-blank. (?)

The visual effect is quite different, as filters now just show. Without ability to edit the styling, users might be taken aback with the sudden visual change.

Additionally, once something is on the map I'd expect to click on it. e.g. to open a tooltip and remove the filter by clicking on the shape.

It works the other direction too. Selecting a filter pill, there's no indication what filter this actually represents (e.g/ by highlighting the shape on the map).

I'm thinking the introduction of #63714 would help bridge the cap. We could include a flag to turn on/off the visual feedback for filters. Then, we have some framework to add more advanced features and functionality over time instead of trying to get it right at first pass.

What do you think?

export function extractFeaturesFromFilters(filters) {
const features = [];
filters
.filter(filter => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if there's another way to determine whether a filter is "spatial". Right now, manually added filters do not show.

Copy link
Contributor

Choose a reason for hiding this comment

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

as discussed, keeping this for a later improvement. Unless users edit DSL manually, ther;es no way in Kibana to add a spatial filter except for using Maps

@jsanz
Copy link
Member

jsanz commented Apr 21, 2020

Maybe we can use a less intrusive styling like a lighter background and dashed outline? Definitely something we may want to expose as a setting, but not sure if it would be for each map individually or a more broad setting at Space or Kibana level.

Apart from the styling, having the filtered outline highlighted when editing would be great, being able to edit it on the map even better!

@nreese
Copy link
Contributor Author

nreese commented Apr 22, 2020

I like it, but I'm wondering if we should introduce this point-blank. (?)
The visual effect is quite different, as filters now just show. Without ability to edit the styling, users might be taken aback with the sudden visual change.

@thomasneirynck Now that map settings has merged, I updated map settings to allow turning feature off and configuring styling of spatial filter styling.

Screen Shot 2020-04-22 at 8 57 43 AM

Additionally, once something is on the map I'd expect to click on it. e.g. to open a tooltip and remove the filter by clicking on the shape.

Great ideas but should be part of another PR. Just showing the filters provides a lot of value and context and we can add to the feature at a later date.

@nreese nreese requested a review from thomasneirynck April 22, 2020 15:01
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 fun, especially in Dashboard context.

Some minor suggestions in-line.

as discussed, map-settings are not taken into account when determining saved/unsaved state. Right now, when a user leaves Maps, it does not give users a warning to save changes. Agreed to do in separate PR.

x-pack/plugins/maps/public/selectors/map_selectors.js Outdated Show resolved Hide resolved
export function extractFeaturesFromFilters(filters) {
const features = [];
filters
.filter(filter => {
Copy link
Contributor

Choose a reason for hiding this comment

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

as discussed, keeping this for a later improvement. Unless users edit DSL manually, ther;es no way in Kibana to add a spatial filter except for using Maps

@nreese nreese requested a review from thomasneirynck April 22, 2020 19:18
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.

🧨

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@nreese nreese merged commit f7ea9b9 into elastic:master Apr 22, 2020
nreese added a commit to nreese/kibana that referenced this pull request Apr 22, 2020
… filters (elastic#63406)

* [Maps] show spatial filters

* pass data into __dataRequests

* extractFeaturesFromFilters

* geo_shape support

* putting it all together

* lower alpha

* update removeOrphanedSourcesAndLayers to avoid removing spatialFiltersLayer

* change array iteration to forEach

* use less precision when distance filter covers larger distances

* fix double import

* add map settings for to configure spatial filters layer

* add map settings alpha slider

* finish rest of map settings

* review feedback

Co-authored-by: Elastic Machine <[email protected]>
nreese added a commit that referenced this pull request Apr 23, 2020
… filters (#63406) (#64250)

* [Maps] show spatial filters

* pass data into __dataRequests

* extractFeaturesFromFilters

* geo_shape support

* putting it all together

* lower alpha

* update removeOrphanedSourcesAndLayers to avoid removing spatialFiltersLayer

* change array iteration to forEach

* use less precision when distance filter covers larger distances

* fix double import

* add map settings for to configure spatial filters layer

* add map settings alpha slider

* finish rest of map settings

* review feedback

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

Co-authored-by: Elastic Machine <[email protected]>
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.

Show bounding box on the map
5 participants