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

Add custom features filter functionality #9584

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

willemarcel
Copy link
Contributor

@willemarcel willemarcel commented Apr 12, 2023

Related to #2676. It adds a new option in the Map Features section that allows the user to set a key=value filter.

You can see it in action in the video below:

id-custom.mov

@tordans
Copy link
Collaborator

tordans commented Apr 17, 2023

This looks like a nice addition. However, I think we should make sure that there are no side effects that might require additional work later … and if someone wants to invest this additional work. I could see this becoming a source of quite a few follow up feature requests and side effects with other features.

Is there a way to test this now without running it locally / is there a preview URL?

A quick list of thoughts:

  • The code changes have a few unrelated re-formattings that are likely uninteded(?)
  • Until now, the list of filter options is exclusive (more or less), so one option does not interact with another option. This changes now… – do we need to handle those cases? Show something to the user? Gray out options that are handled by the custom option already?
  • Which filter I select is stored in the hash URL, right? Is this also the case for the custom filters? Does this maybe break user expectations when sharing a pre configured map URL?
  • This is a power user feature and the input field nudges power user to think of it like the overpass turbo input which allows for more than just one key=value. To clarify, that (for now) this can only do key+value, the UI could be two inputs with an explicit label. That could also be the place where the * case for "any key" is explained.

@willemarcel
Copy link
Contributor Author

@tordans Thanks for your comments.

Is there a way to test this now without running it locally / is there a preview URL?

I've deployed it on https://6446cbf640335e2652b8035f--bright-taffy-b33606.netlify.app

  • Until now, the list of filter options is exclusive (more or less), so one option does not interact with another option. This changes now… – do we need to handle those cases? Show something to the user? Gray out options that are handled by the custom option already?

This is the part that doesn't make me entirely happy. To make it easier to be understood, I think the best solution would be to disable all the other options when the custom is selected, or move it to another section of the UI and make it override the Map Features preferences.

Which filter I select is stored in the hash URL, right? Is this also the case for the custom filters? Does this maybe break user expectations when sharing a pre configured map URL?

The custom filter is also stored in the URL and can be shared. It works the same way the custom background settings work.

@@ -221,15 +221,20 @@ export function rendererFeatures(context) {
return (geometry === 'line' || geometry === 'area');
});


defineRule('custom', function isCustom(tags) {
const [key, value] = prefs('map-features-custom').split('=');
Copy link
Collaborator

Choose a reason for hiding this comment

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

I love this feature, I'm using it in a fork of iD. There is a small bug though, prefs can return null when iD is opened for the first time, which causes a crash here. Something like this would fix it:

Suggested change
const [key, value] = prefs('map-features-custom').split('=');
const [key, value] = prefs('map-features-custom')?.split('=') || [];

@tordans
Copy link
Collaborator

tordans commented Jul 13, 2024

@k-yle what is your experience with this in your fork, is it ready to be merged or are there things we should address?
Specifically the UX issue we talked about above…

I ran into the lacking filtering options again, today and it looks like this PR would be a solution for this.
In my case, I need to get rid of everything that is level=-1 and level=-2 on https://www.openstreetmap.org/edit?note=4139556#map=20/52.51122/13.40564

@k-yle
Copy link
Collaborator

k-yle commented Jul 13, 2024

@tordans there are a few things I think we need to consider first:

  1. This crash
  2. It's not possible to add features in this mode, even if they match the custom filter
  3. Should we allow Regular Expressions? Otherwise filters like level=(1 or 2) would not be possible
  4. Currently, it does not work for vertices, since iD can't render vertices if the parent way is hidden.

I can't think of an easy way to implement the last point, but the other 3 are trivial changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants