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

Map Filter search by resource layer feature #10827

Merged
merged 76 commits into from
Sep 24, 2024
Merged

Conversation

whatisgalen
Copy link
Member

@whatisgalen whatisgalen commented Apr 24, 2024

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Description of Change

Screen.Recording.2024-04-24.at.12.13.11.AM.mov

Overview:

  • map popup sends afeature to a method in map-filter.js called filterByFeatureGeom().
  • the method adds this feature to the map-filter as if a user had drawn a feature.
  • the user can alter the feature on the map to adjust the filter as they want.
  • clicking a new feature removes the old one from the filter, replacing it with the new

Issues Solved

#10816

Checklist

  • Unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Ticket Background

Further comments

blocked until PR #11056 gets merged (done)

@whatisgalen whatisgalen changed the title 10816 popup map filter Map Filter search by resource layer feature Apr 24, 2024
@whatisgalen
Copy link
Member Author

My new test is failing, debugging that. In the meantime the feature is definitely testable.

<span data-bind="text: $root.translations.edit"></span>
</a>
<!--/ko-->
<a data-bind="click: function() {sendFeatureToMapFilter($data, $index())}, visible: showFilterByFeature($data, $index())" href="#">
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, it seems like featureGeomIndex is always zero, regardless of which geometry is selected. This is true if there are 2 geojson nodes, or 2 features in a single geojson node.

let foundFeature = null;
for (let geometry of popupFeatureObject.geometries()) {
if (geometry.geom && Array.isArray(geometry.geom.features)) {
foundFeature = geometry.geom.features.find(feature => feature.id === popupFeatureObject.featureid);
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 seeing in my instance that feature.id is in the format 9caa54cd72738367ea32eda99400c385 and popupFeatureObject.featureid is in the format 9caa54cd-7273-8367-ea32-eda99400c385 so this is never returning a geometry.

Seems like the feature.id is stored without the hyphens in the tiles.tiledata column.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think popupFeatureObject.feature.properties.featureid should be used insetead of 'popupFeatureObject.featureid. Using 'popupFeatureObject.featureid always results in the first geometry in the resource being selected.

arches/app/media/js/utils/map-popup-provider.js Outdated Show resolved Hide resolved
arches/app/media/js/utils/map-popup-provider.js Outdated Show resolved Hide resolved
Copy link
Contributor

@bferguso bferguso left a comment

Choose a reason for hiding this comment

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

This is looking really good!

I came across one issue - I created a bunch of geometries using the trigger views to do some load testing, and the trigger views don't add the featureid to the tiledata. We'll need to populate that value from the trigger views to allow geometries to be added that way.

The featureid is there for geometries created through the system. Wondering if:

  1. There should be a migration that adds any missing featureids if they're not present.
  2. The trigger views need to be updated to populate featureid if populating geometries that way.

I don't use CSV import or bulk importer, but wondering if those processes do/do not populate featureid as part of those processes.

I'll manually add the featureid to the tiles that are missing it, but I think the missing values would be problematic for users with geometries not created with the UI.

@@ -667,6 +667,7 @@
map-add-line='{% trans "Add line" as mapAddLine %} "{{ mapAddLine|escapejs }}"'
map-add-polygon='{% trans "Add polygon" as mapAddPolygon %} "{{ mapAddPolygon|escapejs }}"'
map-select-drawing='{% trans "Select drawing" as mapSelectDrawing %} "{{ mapSelectDrawing|escapejs }}"'
filter-by-feature='{% trans "Add Feature to Map Filter" as filterByFeature %} "{{ filterByFeature|escapejs }}"'
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently only one map geometry or feature can be used as a geometry filter. Perhaps this should read "Filter by Map Feature" instead? The term "Add" is somewhat confusing since it will clear any existing geometry filters.

</a>
<!--/ko-->
<a data-bind="click: function() {sendFeatureToMapFilter($data);}, visible: showFilterByFeature($data)" href="#">
<i class="fa fa-map-pin"></i>
Copy link
Contributor

Choose a reason for hiding this comment

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

What about using a funnel icon instead? (ion-filter icon) Or a magnifying icon, which is used as search elsewhere (fa fa-search)?

@chiatt
Copy link
Member

chiatt commented Sep 24, 2024

@bferguso It looks like @whatisgalen addressed the change you requested. There was a scoping issue that popped up after that fix (which I think you may have observed) but Alexei fixed that in a90dfaa. I also noticed that the filter button would appear in the map widget within the resource editor. Alexei was able to take care of that as well.

Copy link
Contributor

@bferguso bferguso left a comment

Choose a reason for hiding this comment

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

Looks good!

@chiatt chiatt merged commit c558483 into dev/7.6.x Sep 24, 2024
7 checks passed
@chiatt chiatt deleted the 10816_popup_map_filter branch September 24, 2024 01:31
@whatisgalen
Copy link
Member Author

whatisgalen commented Oct 31, 2024

Per my comment, the featureid was removed from the request payload to the backend (map-filter) because if the user edited the drawn polygon on the map, (for example to cover a larger spatial query area) the feature identified by featureid would differ from the query geometry, and there wasn't a great way to confirm whether the geometry had actually changed (given potential precision differences). This would have yielded search results out of sync with the search query.

However, because the only payload is the geojson feature itself, this means large coordinate sets can exceed the character limit on the GET request header (and url length in the browser). Unfortunately, those requests then fail, hence #11578 and the need for #11582

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.

Feature Idea: Map Feature popup has link to use in Map Filter
4 participants