KeepRight interface followup issues

bhousel commented 5 years ago

Breaking out some of @tordans's feedback into a separate issue. I think all the other ones from the comment are fixed already, such as fixing the layer order to make it easier to click on things, offsetting the markers slightly from the geometry they are about, and improving the color scheme and contrast of the markers, but let me know if I cut anything important.

it took me quite a while to understand what the issue is related to, since I did not understand the "way 35557143" (left panel) as the part that is most relevant for this.

[x] Yes, I'd like to make some of the embedded names nicer instead of showing entity ids.

d. Is there a validation on the comment field? I feel like it should validate to at least 2 word for ignore cases, right? I dont want to test it since I dont know what will happen and the data are live, right?

[x] KeepRight allows only a single anonymous comment. You can change it or remove it, but there's not much else to know about commenting. I think I'm ok with just letting people play with the comment field, but open to ideas on ways to make it more clear for users.

e. I image when I could actually fix the issue, I have the same problem as with https://github.com/openstreetmap/iD/issues/5170, I would like to manually sync my commit message with the get-right-issue. This is the same for HOT Task manager btw, I type my change twice, once in the task manager, once in the commit message. Could the QA layer maybe pre-fill the commit-message with a hash-tag like "#keepright #keepright-issue-123" so there is some link?

[x] Yes this is a good suggestion. I'm wondering whether we should introduce a dedicated changeset tag to hold all the issues that users are closing?

f. How do I get from one issue to the next one? Lets say I solved this one, how do I find the next one?

[ ] There is parallel work going on where we are introducing an Issue Manager for both server-side and client-side validation issues.. The KeepRight issues will probably end up in here.

g. I am looking for a way to share a specific keepright-issue on iD editor. I can share the Keepright-URL, which is provided in iD, but what I was hoping/looking for is: Once I select an KR-issue, the iD URL changes to reflect this issue.

[ ] Yeah we should consider this - there are lots of other requests to use URL parameters to show different layers and focus on things in those layers (e.g. Mapillary/OpenStreetCam detections too)

Originally posted by @tordans in https://github.com/openstreetmap/iD/pull/5201#issuecomment-417834268

kymckay commented 5 years ago

Regarding

it took me quite a while to understand what the issue is related to, since I did not understand the "way 35557143" (left panel) as the part that is most relevant for this.

I envision something along the lines of how relation members are listed working well here - especially with the same highlighting to show the entities

bhousel commented 5 years ago

I added some code to replace the identifier with a friendlier name if possible. (This is not always possible because you might be viewing the markers at a lower zoom and the map data hasn't been loaded around there).

It will also highlight the feature if you hover over the link, and zooming to the entity works a little better now.

kymckay commented 5 years ago

Another suggestion:

The left panel should also have a link somewhere to the object the issue as actually attached to as it may not always be obvious which it is (currently it only links to additional entities listed in some descriptions).

It would also serve as a good way to zoom to the error when browsing them from a low zoom level.

thomas-hervey commented 5 years ago

The left panel should also have a link somewhere to the object the issue as actually attached to as it may not always be obvious which it is (currently it only links to additional entities listed in some descriptions).

[x] @SilentSpike, I agree and this is an easy fix. This was in my original implementation but I think it may have been hidden when @bhousel worked on cleaner naming and descriptions in https://github.com/openstreetmap/iD/pull/5201/commits/2ec02f369f02626cd94c0a47d9ebb698c8fca431 and https://github.com/openstreetmap/iD/pull/5201/commits/9557c0204a2c024c1561450e799a7c1f0264fd99. I think it should be re-enabled.

bhousel commented 5 years ago

@SilentSpike, I agree and this is an easy fix. This was in my original implementation but I think it may have been hidden when @bhousel worked on cleaner naming and descriptions in 2ec02f3 and 9557c02. I think it should be re-enabled.

I'm not sure that we get the geometry that the issue is attached to from KeepRight.. For example in this one, the original message was "This node is very close but not connected to way #11775974". KeepRight doesn't give us the "this node" part. (we could probably guess 🤔)

thomas-hervey commented 5 years ago

@bhousel, if I understand correctly, we do get the geometry that the issue is attached to. It's called _objectid

Before the header was changed, this object id was visible and clickable. You can see examples in @SilentSpike's comment https://github.com/openstreetmap/iD/pull/5275#issuecomment-418100026 I'll have to dig up a commit I made to see where the object id gets used.

bhousel commented 5 years ago

@bhousel, if I understand correctly, we do get the geometry that the issue is attached to. It's called _objectid

Nice! Thanks @thomas-hervey.. Sorry if I removed it - I will find a way to put them back..

kymckay commented 5 years ago

Yeah an object ID and type is being served in the json, whether or not it links to the specific element for evey error type I don't know though

bhousel commented 5 years ago

Yeah an object ID and type is being served in the json, whether or not it links to the specific element for evey error type I don't know though

Yep - I'm updating all the strings today to support adding links to the "this whatever" part of the message.

In a few cases, I'm rewriting the strings to make this easier to do. For example, one of the errors we get just says "missing maxspeed tag", so in code I'm changing that to "This highway is missing a maxspeed tag" (to give us something to actually add a link to).

bhousel commented 5 years ago

I redid all the error messages to make sure they contain links to the object. This unfortunately means a lot more translation work, but the error messages are a lot better now!

I also made a few other small improvements to the links. If the user clicks a link to an entity that is not yet loaded into the graph (maybe they are zoomed out too far), we now load the entity from OSM before entering select mode.

kymckay commented 5 years ago

Just gave it a test and a very nice improvement 👍

I notice the URL errors (411, 412, 413) haven't been given this treatment, was that intentional?

bhousel commented 5 years ago

I notice the URL errors (411, 412, 413) haven't been given this treatment, was that intentional?

oh you're right! I forgot those ones.. I will add them.

tordans commented 5 years ago

This looks great, thanks. I really like the highlighting (https://github.com/openstreetmap/iD/issues/5679#issuecomment-452081886).

JFYI: Small design feedback at https://github.com/openstreetmap/iD/pull/5695 (padding) and https://github.com/openstreetmap/iD/issues/5169#issuecomment-453484830 (zoom-to-icon + zoom-level).

kymckay commented 5 years ago

More of a QOL improvement:

If you have relevant map features disabled and then click on the link to a feature in the error description, you have now selected a feature which isn't visible. Perhaps the currently selected feature should always be made visible?

edit: not directly a keepright issue, but something that is more exposed as more Q/A is added in (including more links to entities that may be currently hidden).

Mahabarata commented 5 years ago

Hi,

I don't want to open a new issue, maybe this is the good place for a suggestion about Keepright. With the last release, I really love the add of Keepright in ID : very easy to use, it is a very good improvment of ID, you have made a very good job, thanks a lot. But when I check the box then close my browser, when I come back Keepright is unchecked : the first time I was happy because I did not see any more "bugs" in OSM in the area where I was working and then I saw the unchecked box. It will be great if the checked box could stay. But in fact no : I think that Keepright is a tool everyone should use (or at least that everyone should see the bugs shown by this tool) so I think it will be better to have no choice : no box to check but Keepright always there. Because I think that a lot of people have not seen this box and don't know of your great improvment. Why did you hide it, if you know what I mean ?

Best regards

quincylvania commented 5 years ago

@Mahabarata Good suggestion about persisting the KeepRight visibility setting. We should probably persist more of the other settings as well, like photo overlay and OSM note visibility.

This drawback to always showing KeepRight issues is that it could be overwhelming to new users and cumbersome for power-mappers. However, the next release of iD will include #5830 with numerous improvements to iD-native validation, such as flagging KeepRight-like issues as you edit.

Mahabarata commented 5 years ago

I understand what you mean, I just find this kind of result for the first time : 2019-02-15_155056 So forget my suggestion of always showing KeepRight, sorry for this bad idea. Best regards

openstreetmap / iD

KeepRight interface followup issues #5679