Feat: Click to switch features that are crossing ways

[Waqibsk]

Fixes #11328

This PR , turns the labels of the crossing features into a link which when clicked on would select that feature.
This makes it easier for users to navigate between crossing features.

Screenshot:

[tordans]

Functionally I think this is a good idea. Rapid has a „highlight on hover“ for their MapRoulette integration which I find even more useful because it does not change the current selection. I looked into this for #11310 but iD might be missing something to use this easily.

Visually I think we should have a look if there is any existing „Text as action“ style that we can reuse like the blue color or something else.

Semantically the span should be a button tag IMO.

[tyrasd]

highlight on hover

do you mean similar to the blue outline that is currently already shown for the secondary affected feature of the validation?

(The code responsible for this is utilHighlightEntities in util/util.js.)

Or did you mean something different? 🤔

because it does not change the current selection

Note that just identifying the features mentioned in the validation message is not mainly what this PR tries to solve: The main use case is to be able to quickly go to the other feature in case fixing the issue is more easily be done by modifying the other feature.

[tyrasd]

Semantically the span should be a button tag IMO.

Something like this?

Hmm, I don't think that looks great. I'd rather go with a simple link (<a>), to visually mark the text as individually clickable (i.e. blue text, correct cursor; avoiding the use of inline css). This is a common UX pattern used in iD and there are even already a couple of places in iD where <a> links lead one directly to select a specific map feature (e.g. in Osmose QA issues).

[tordans]

highlight on hover

do you mean similar to the blue outline that is currently already shown for the secondary affected feature of the validation?
…
(The code responsible for this is utilHighlightEntities in util/util.js.)

Yeah, that's it, thanks. Now that I think about it again, the different to Rapid was something else … but not relevant here.

because it does not change the current selection

Note that just identifying the features mentioned in the validation message is not mainly what this PR tries to solve: The main use case is to be able to quickly go to the other feature in case fixing the issue is more easily be done by modifying the other feature.

👍

Semantically the span should be a button tag IMO.

Something like this?

No, I was only referring to the semantics, not the visual representation, mainly from an accessibility and general clean-html point of view where actions elements should be either a tags or button tags (with a tags reserved for something that has a href and button tags for something that has a onclick or form submit), but not span or div. Visually both can be styles as the other, which is based on a completely different set of guidelines.

So +1 for the link style look of the second screenshot.

[tyrasd]

Hi @Waqibsk . This is a good start. The following would be to be adjusted still:

• feature1 is already the selected feature, thus it does not need to be clickable
• as mentioned above: it would be better if the clickable part of feature2 is a <a> element, instead of a custom styled <span>
• currently, this only adds the link for the crossing ways validation. I believe other some of the other validation modules also have warnings involving more then one feature. Could you please add the same link also to those validators?

I noticed that the implementation is quite a bit more convoluted than expected (and using selection.html which should be avoided in general). I see why you chose to approach the implementation that way, as the localizer module did not have a functionality to accept a callback function to specify additional html markup for the replacement strings. I did now extend the functionality to add this missing functionality in #11347. Please take a look and rebase your PR on that branch, as it should allow you to greatly reduce the overhead in this PR. I think you should end up with something like the following:

return entity1 && entity2
    ? t.append('issues.crossing_ways.message', {
        feature: utilDisplayLabel(entity1, graph, featureType1 === 'building'),
        feature2: selection => selection
            .append('a')
            .classed('feature-link', true)
            .text(utilDisplayLabel(entity2, graph, featureType2 === 'building'))
            .click(…)
    }) : '';

[Waqibsk]

feature1 is already the selected feature, thus it does not need to be clickable

I think currently, the way iD handles crossing ways validation, the order of the features doesn’t change after switching. For example, if a railway track (feature1) is crossing a river (feature2), and the railwayis selected first, then after clicking the river link, the railway still remains in the “first” position . Because of this, the user cannot switch back to the railway unless both features are made clickable.
That said, if we prefer not to make the currently selected feature clickable, a conditional could be added to handle this

[tyrasd]

the order of the features doesn’t change after switching.

True. But still: only the not already selected feature should be clickable, regardless of if it's the first or second feature. Should none of the selected features exactly match the current selection (e.g. when multiple features are selected), then both links can be clickable.

[Waqibsk]

Screenshots:

[matkoniecz]

do we need that new empty line?

[matkoniecz]

it seems that something weird went with indentation here

[matkoniecz]

no proper code review yet (but I spotted some very minor things - given how long PR waited I can also fix them myself), but I tested and it works beautifully - thanks!

[k-yle]

this is blocked by #11347, and will conflict with #9888

so i think #11347 needs to be merged first¹

Footnotes

1. #9888 could also be merged first, or last. doesn't really matter ↩

[matkoniecz]

this is blocked by #11347

interesting, seemed to work entirely fine in my testing

[k-yle]

yeah it works fine, but iirc, the only purpose of #11347 was to make the implementation in this PR cleaner

[matkoniecz]

yeah it works fine, but iirc, the only purpose of #11347 was to make the implementation in this PR cleaner

and it can be now done here!

[matkoniecz]

@Waqibsk #11347 was merged, can you update this PR to use code improvements enabled by it?