Add a button in the mapillary image photo viewer to "add the current image id as field to feature" #10046

laigyu · 2023-12-20T16:20:38Z

edit : this pr only focus on button in viewer

more info see openstreetmap#6196 (comment) I do the b. (set button) and c. (view button) order: click OSM object -> click mapillary layer will close the OSM input field. need to do in reverse order

It is the a. part in openstreetmap#6196 (comment) still need to add button disabled logic

tyrasd

Hey. This is awesome!

There are only a few minor-ish things I would change:

The icon used inside of the mapillary viewer is not all too intuitive: One might assume that it refreshes the image or something like that. Maybe the load icon would be better? Or the operation-extract one? Or the + from operations-merge?
The used strings should be made independent from the photo service provider to be able to be used them in the future with the other street level photo providers as well. See suggestions below.
The button to set the mapillary tag should be disabled or hidden if no OSM entity is selected.
The button to open the viewer should be disabled if the photo is already currently displayed
If the mapillary id of an OSM object is invalid or does not exist, the viewer is opened showing the last selected photo (or remains open if it was already showing a photo). It would be better if the viewer would show some kind of error state in this situation.

data/core.yaml

refactor the code expandability for other photo_overlay Not sure about context.features().on('change'), and hash part

1 problem remain: if entity select and close the photoviewer, view button will not update class, still clickable though

laigyu · 2024-01-24T16:04:39Z

set button in field and photoviewer are both use to set current id to input,
not sure about how to do

is fine to have both and use same logic as field set button to disable photoviewer set button
if mapillary id field not show,show photoviewer set button and after click it ,remove the button ,and use only field set button to update id
only use photoviewer button and not use set button in field
or other better way ? 🙏

and update input.js

button disable class reacts to photoviewer close

'_' change to '-'

tordans · 2024-04-05T03:39:08Z

@laigyu why did you close this PR? I was looking forward to seeing it as part of iD at some point?
Would you be up to polish it and bring it over the finish line?
(This does take a while … but that is how it is with projects like iD…)

…pillary

tordans · 2024-04-05T07:09:29Z

@laigyu I tested the UI and I suggest you split of a few things just to make this easier to merge an iterate on.

The part that is most clear to me is his use case:

UseCase A: Add the Mapillary field. And if an mapillary image is selected, provide an action to copy the iD into the field.

This part of the UI and interaction looks good to me with some adjustments

Change the icon of the button
Fix the border radius of the button

icon

I assume we are using font awesome free icons here, right?

I looked at some options, here are my picks

icon	link	comment
	https://fontawesome.com/icons/eye-dropper?f=classic&s=solid	my favorite. it tells the "pick this" story well
	https://fontawesome.com/icons/right-to-bracket?f=classic&s=solid
	https://fontawesome.com/icons/arrow-right-to-bracket?f=classic&s=solid
Cell	https://fontawesome.com/icons/file-import?f=classic&s=solid	not really
Cell	https://fontawesome.com/icons/file-arrow-down?f=classic&s=solid	not really

border radius

It looks like, this CSS needs to change:

.ideditor[dir='ltr'] .form-field-input-identifier > input:last-child,
.ideditor[dir='rtl'] .form-field-input-identifier > input:first-child,
- .ideditor[dir='ltr'] .form-field-input-identifier > button {
+ .ideditor[dir='ltr'] .form-field-input-identifier > button:last-child {
    border-bottom-right-radius: 4px;
}

so the radius is only applied to the last button

I suggest to make a PR that only includes this change, which should be much easier to merge.

tordans · 2024-04-05T07:13:33Z

About the other usecases…

UseCase B: Show current mapillary image from the sidebar

I think this is a nice idea, but it has a lot of edge cases that are hard to get right. I suggest to work on this last, because there is a feature to show the current image already. It is not as ideal, because it opens it in an new tab, but the other use cases are more important IMO.

This is what it looks like now

The issues I see is, that one can show an image but that show click does not activate the mapillary overlay, it only shows the current image. Apart from that, I think the Icon and interaction is good.

There is also this issue from #10046 (review)

If the mapillary id of an OSM object is invalid or does not exist, the viewer is opened showing the last selected photo (or remains open if it was already showing a photo). It would be better if the viewer would show some kind of error state in this situation.

tordans · 2024-04-05T07:23:45Z

About the other usecases

UseCase C: Add a `mapillary` field with the current image id to the selected feature from the current image UI

This feature is super nice!

I think those things should change…

Use the same icon as for the field, eg. the "picker" icon. I think we should reuse the icon because it does the same thing, it copies the id inside the field. The context of the icon placement clarifies the direction of the action well enough, so no need for a different icon IMO. (Also, those actions are super abstract, so it's hard to find a good icon…)
Add a tooltip. It looks like iD has a tooltip system and I hope that can be reused in this case, so users see the icon title attribute as a tooltip which will explain the feature a lot better.
Example tooltip
Move the button into the panel and change the style.
Style: I suggest to make it look like any button in the sidebar: (with full rounded corners)
Position: I suggest here
Only one button per feature. Right now there are situations when multiple buttons are present:
Example:
Both buttons seem to update the same tag, however.
There should be one button only per selected feature. When more than one feature is selected (multiselect) the button should be either inactive or it should add the same image id to both features.

laigyu · 2024-04-05T15:16:10Z

so there should be only two button right ?

tordans · 2024-04-06T04:12:49Z

so there should be only two button right ?

UseCase A adds a button in the sidebar mapillary field to "fetch the current image id"
UseCase C adds a button in the mapillary image panel to "add the current image id as field to feature"
UseCase B adds another button in the sidebar mapillary field to "open current image in preview" (that would be three buttons on this field now) – However, as suggest I would extract this into a separate PR to get UseCase A+C merged faster.

laigyu · 2024-04-07T15:04:50Z

I remove a and b part , only left C part

Use the same icon as for the field, eg. the "picker" icon. I think we should reuse the icon because it does the same thing, it copies the id inside the field. The context of the icon placement clarifies the direction of the action well enough, so no need for a different icon IMO. (Also, those actions are super abstract, so it's hard to find a good icon…)

I think the svg is from svg
I dont know is it possiable to add your suggestion

Add a tooltip.

I think already has tooltip. currently is .attr('title', t('inspector.set_photo_from_viewer')); ( no triangle )
the other is .call(uiTooltip().title(() => t.append('inspector.set_photo_from_viewer'))) just like your picture show
first type is match the right side close button (X)

Move the button into the panel and change the style

I think position is already at the top left corner. but I dont know why your pic is not as same as mine. plz check if still wrong
as for round style I copy the style from close button right. shall we change it ?

Only one button per feature. Right now there are situations when multiple buttons are present:

I see your pic show two button + + , could you give me step to reproduce it

functionality : currently the button is listen to imagechanged event.
so button will not show at the beginning .
to trigger the imagechange . need to click < > at the top black bar or white arrow first.
as for other , I think is good .

Any suggestions are welcome.Thank you

tordans · 2024-04-08T05:14:25Z

I think reducing the footprint of this feature is good thing. Some feedback:

I think the svg is from svg I dont know is it possiable to add your suggestion

I thing this svg is the wrong folder it is https://github.com/openstreetmap/iD/tree/develop/svg/iD-sprite which tyrasd referenced in #10046 (review)
The readme there suggests that adding files is possbile.
But … lets stick with the + icon for now an see what tyrasd thinks about it.

The + is used in other places in the sidebar like the name-tag-translation feature, so that does fit.

Add a tooltip.

I think already has tooltip. currently is .attr('title', t('inspector.set_photo_from_viewer')); ( no triangle ) the other is .call(uiTooltip().title(() => t.append('inspector.set_photo_from_viewer'))) just like your picture show first type is match the right side close button (X)

If possible, I really think we should use the black tooltip for this.
For the X it really is more of an accessibility feature. But here we need it for people to understand what the feature is (the x is clear)

Move the button into the panel and change the style

I think position is already at the top left corner. but I dont know why your pic is not as same as mine. plz check if still wrong as for round style I copy the style from close button right. shall we change it ?

It is working now. I opened #10193 about the technical issue behind it.

Only one button per feature. Right now there are situations when multiple buttons are present:

I see your pic show two button + + , could you give me step to reproduce it

This is gone now, as well. No idea why the npm run build fixed that…

Btw, selecting multiple feature and using the feature also works like it should.

open issue 1: show button once feature is selected

functionality : currently the button is listen to imagechanged event. so button will not show at the beginning . to trigger the imagechange . need to click < > at the top black bar or white arrow first. as for other , I think is good .

Yes, that is an issue that needs to be fixed somehow.

open issue 2: hide button once no feature is selected (anymore)

The other issue is, that the + does not go away when I unselect a feature.
This sounds to me like you will also need to listen to some "feature un/selected" event. But I don't know enough about this part of the code.
Should you not find anything, I suggest to ping tyrasd.

open issue 3: disable, not hide button

In #10046 (review) the suggestion is to disable the buttons, not hide them. (They need a visual disabled state for that; maybe a cursor: not-allowed;?)

The button to set the mapillary tag should be disabled or hidden if no OSM entity is selected.

The button to open the viewer should be disabled if the photo is already currently displayed

laigyu · 2024-04-12T10:41:27Z

besides button svg, I think is good.

tordans

I looks like I never clicked "send" on those wording suggestions.

tordans · 2024-04-05T07:28:21Z

data/core.yaml

@@ -788,6 +788,10 @@ en:
      inch: in
    max_length_reached: "This string is longer than the maximum length of {maxChars} characters. Anything exceeding that length will be truncated."
    set_today: "Sets the value to today."
+    set_photo_from_viewer: "Store this photo on the currently selected map object"


Suggested change

set_photo_from_viewer: "Store this photo on the currently selected map object"

set_photo_from_viewer: "Tag this photo id on the currently selected map object"

"Store" suggest an upload / file. But it is only the id that is referenced as a tag. Maybe like this?

tordans · 2024-04-05T07:28:49Z

data/core.yaml

@@ -788,6 +788,10 @@ en:
      inch: in
    max_length_reached: "This string is longer than the maximum length of {maxChars} characters. Anything exceeding that length will be truncated."
    set_today: "Sets the value to today."
+    set_photo_from_viewer: "Store this photo on the currently selected map object"
+    set_photo_from_field: "Use the currently displayed photo"


Suggested change

set_photo_from_field: "Use the currently displayed photo"

set_photo_from_field: "Use the id of the currently displayed photo"

tordans · 2024-04-05T07:29:49Z

data/core.yaml

@@ -788,6 +788,10 @@ en:
      inch: in
    max_length_reached: "This string is longer than the maximum length of {maxChars} characters. Anything exceeding that length will be truncated."
    set_today: "Sets the value to today."
+    set_photo_from_viewer: "Store this photo on the currently selected map object"
+    set_photo_from_field: "Use the currently displayed photo"
+    show_photo_from_field: "Open image in viewer"


Suggested change

show_photo_from_field: "Open image in viewer"

show_photo_from_field: "Open image in street-level photo viewer"

Just to make it a bit more precise.

tordans · 2024-04-15T04:47:58Z

@laigyu this looks great, thanks. And @tyrasd I think this is ready for another review from you.

Maybe @tyrasd can polish off the wordings / resolve the wording suggestions?

There is a minor thing that gave me pause: Whenever I select an image which was already added as tag, the image PLUS is disabled. However, there is no help in the UI that explains why it is disabled.

What do do…

I think it is OK to leave it as is
I think it would be OK to activate the button which would just add the same tag again (change nothing, but allow clicking)
More complex solution would be a separate error message in the tooltip "This image is already tagged on this feature."

laigyu · 2024-04-16T08:23:36Z

@tordans I think we dont need usecase a button,which is offer the same functionanality as the usecase c button,
the diff is that a is in field, c is in photoviewer, or did I miss something?

(this one is also used in other similar UI locations, e.g. the "add tag" in the raw tags editor, etc.)

code: we should not to not rely on directly manipulating/accessing other components DOM elements

resolved

tyrasd · 2024-07-12T16:13:01Z

Sorry for the long wait on this PR. I've in the meantime fixed one last(?) bug: The "add mapillary tag" button was not shown if a map feature was newly created while a street level photo was open¹.

Also, I refactored some parts of the code: It's not good practice to directly rely on other modules' DOM elements, as the implementation details could change at any moment and would break the functionality here. The implementation now directly manipulates the respective OSM entities² instead.

There is now also a limit of (an arbitrary) 100m to the distance between the map feature and the photo to prevent accidentally adding a mapillary tag to a far away map feature.

Finally, I've made minor tweaks to the icon (slightly smaller + which matches the styling used on other locations in the UI where we use a plus to indicate that a tag is going to be added) and tooltip texts.

@tordans maybe you would like to give it a final look-over to check whether this didn't introduce any new bugs? 🙇

use case: when mapping a missing business from a mapillary image one would like to immediately set the photo as a "source" of the newly created osm object. ↩
read more about iD's data model ↩

tordans

@tyrasd thanks for the update <3. I checked the frontend / UX just now and all works great.

There are things to optimize, but I am not sure they are relevant enough as to hold this back…

when I add an image with +, then move up/down the image-path (AKA next/prev image), I cannot overwrite the image (anymore?) – this is something I see myself doing. However, I can still remove the image from the field first, then reselect the feature, then add with +
- note that the same does work, when I change to a different image, so both cases (the one below) might actually be the same
add image with +, then use the undo or trash icon on the mapillary field to remove the image id again. Now the plus will be disabled with the "already_set" reason until I un-and-reselect the feature. — I don't think this is a relevant use case, so I would leave it as is.
The same issue is true for the undo action: use + to add the image id to a building, use cmd+z to undo the change => now i cannot re-add the image, or the next/prev image (which would be my use case) but instead have to reselect the building first.

addresses openstreetmap#10046 (review)

tyrasd · 2024-07-16T20:14:11Z

undo or trash icon […] undo action

good catch, this should now work as expected

next/prev image

this works for me (already before today's fix commit)?! maybe I'm overlooking an edge case where this is not updating properly? If this still exists: could you maybe open a new issue and list the individual steps to reproduce it?

laigyu added 2 commits December 20, 2023 20:05

Add set button and view button at mapillary field (openstreetmap#9339)

87f0014

more info see openstreetmap#6196 (comment) I do the b. (set button) and c. (view button) order: click OSM object -> click mapillary layer will close the OSM input field. need to do in reverse order

fix setButton when imageChange disabled

a2296cf

laigyu closed this Dec 20, 2023

laigyu reopened this Dec 20, 2023

laigyu added 2 commits December 21, 2023 21:49

fix button title

33f4f8e

Add set mapillary id button in the mapillary photo viewer

5890b79

It is the a. part in openstreetmap#6196 (comment) still need to add button disabled logic

tyrasd added usability An issue with ease-of-use or design streetlevel An issue with streetlevel photos labels Jan 17, 2024

tyrasd previously requested changes Jan 17, 2024

View reviewed changes

data/core.yaml Outdated Show resolved Hide resolved

laigyu added 4 commits January 18, 2024 15:20

Update icon to '#iD-operation-merge'

6523ae6

update core.yaml

2b0e0f6

Update set_photo_from_viewer button create/remove logic

0ef947d

refactor the code expandability for other photo_overlay Not sure about context.features().on('change'), and hash part

update open the viewer button logic

437ecf9

1 problem remain: if entity select and close the photoviewer, view button will not update class, still clickable though

laigyu added 5 commits January 27, 2024 15:14

Add error message in photoviewer when invalid id

0ad46e0

and update input.js

finish open the viewer button logic

cc10898

button disable class reacts to photoviewer close

refactor the code

b597cc1

code style change

666bbbf

refactor the code

6b860b7

'_' change to '-'

laigyu closed this Mar 3, 2024

tordans mentioned this pull request Apr 5, 2024

Add panoramax streetlevel image data #9941

Closed

laigyu reopened this Apr 5, 2024

Merge remote-tracking branch 'upstream/develop' into add-button-to-ma…

8bb1d0d

…pillary

revert field button

94e32a8

revert error handler

d448b58

tordans mentioned this pull request Apr 8, 2024

npm run start does not pick up css changes #10193

Closed

laigyu added 2 commits April 8, 2024 18:22

tooltip style change

6b8b41c

update butotn logic

2325de4

laigyu changed the title ~~Add set button and view button at mapillary field (#9339)~~ Add a button in the mapillary image photo viewer to "add the current image id as field to feature" Apr 12, 2024

tordans reviewed Apr 15, 2024

View reviewed changes

tooltip update

b1ff86c

laigyu and others added 6 commits April 17, 2024 22:59

Merge branch 'openstreetmap:develop' into add-button-to-mapillary

904c802

use slightly smaller "plus" icon

6e9d225

(this one is also used in other similar UI locations, e.g. the "add tag" in the raw tags editor, etc.)

slightly tweak tooltip text

2ea9691

make button work on newly created features; refactor code

b604ef8

code: we should not to not rely on directly manipulating/accessing other components DOM elements

disable button when photo is too far from selected feature

d9c973b

lint

1d0f9e6

simplify code

7d65b19

tyrasd requested a review from tordans July 12, 2024 16:13

add to changelog

daf2c98

tordans mentioned this pull request Jul 13, 2024

Action to quickly hide the arial imagery lines #10321

Open

tordans approved these changes Jul 13, 2024

View reviewed changes

update "add mapillary tag" button state when tags of the feature change

d13c202

addresses openstreetmap#10046 (review)

tyrasd merged commit 984f25a into openstreetmap:develop Jul 16, 2024
3 checks passed

tordans mentioned this pull request Nov 3, 2024

Add panoramax=* universal field openstreetmap/id-tagging-schema#1344

Merged

tordans mentioned this pull request Dec 19, 2024

Panoramax: Adopt "copy id" feature from mapillary #10611

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a button in the mapillary image photo viewer to "add the current image id as field to feature" #10046

Add a button in the mapillary image photo viewer to "add the current image id as field to feature" #10046

laigyu commented Dec 20, 2023 •

edited

Loading

tyrasd left a comment

laigyu commented Jan 24, 2024

tordans commented Apr 5, 2024

tordans commented Apr 5, 2024 •

edited

Loading

tordans commented Apr 5, 2024 •

edited

Loading

tordans commented Apr 5, 2024 •

edited

Loading

laigyu commented Apr 5, 2024

tordans commented Apr 6, 2024

laigyu commented Apr 7, 2024

tordans commented Apr 8, 2024 •

edited

Loading

laigyu commented Apr 12, 2024

tordans left a comment

tordans Apr 5, 2024

tordans Apr 5, 2024

tordans Apr 5, 2024

tordans commented Apr 15, 2024 •

edited

Loading

laigyu commented Apr 16, 2024

tyrasd commented Jul 12, 2024

tordans left a comment

tyrasd commented Jul 16, 2024

	set_photo_from_viewer: "Store this photo on the currently selected map object"
	set_photo_from_viewer: "Tag this photo id on the currently selected map object"

	set_photo_from_field: "Use the currently displayed photo"
	set_photo_from_field: "Use the id of the currently displayed photo"

	show_photo_from_field: "Open image in viewer"
	show_photo_from_field: "Open image in street-level photo viewer"

Add a button in the mapillary image photo viewer to "add the current image id as field to feature" #10046

Add a button in the mapillary image photo viewer to "add the current image id as field to feature" #10046

Conversation

laigyu commented Dec 20, 2023 • edited Loading

tyrasd left a comment

Choose a reason for hiding this comment

laigyu commented Jan 24, 2024

tordans commented Apr 5, 2024

tordans commented Apr 5, 2024 • edited Loading

UseCase A: Add the Mapillary field. And if an mapillary image is selected, provide an action to copy the iD into the field.

icon

border radius

tordans commented Apr 5, 2024 • edited Loading

UseCase B: Show current mapillary image from the sidebar

tordans commented Apr 5, 2024 • edited Loading

UseCase C: Add a mapillary field with the current image id to the selected feature from the current image UI

laigyu commented Apr 5, 2024

tordans commented Apr 6, 2024

laigyu commented Apr 7, 2024

tordans commented Apr 8, 2024 • edited Loading

open issue 1: show button once feature is selected

open issue 2: hide button once no feature is selected (anymore)

open issue 3: disable, not hide button

laigyu commented Apr 12, 2024

tordans left a comment

Choose a reason for hiding this comment

tordans Apr 5, 2024

Choose a reason for hiding this comment

tordans Apr 5, 2024

Choose a reason for hiding this comment

tordans Apr 5, 2024

Choose a reason for hiding this comment

tordans commented Apr 15, 2024 • edited Loading

laigyu commented Apr 16, 2024

tyrasd commented Jul 12, 2024

Footnotes

tordans left a comment

Choose a reason for hiding this comment

tyrasd commented Jul 16, 2024

laigyu commented Dec 20, 2023 •

edited

Loading

tordans commented Apr 5, 2024 •

edited

Loading

tordans commented Apr 5, 2024 •

edited

Loading

tordans commented Apr 5, 2024 •

edited

Loading

UseCase C: Add a `mapillary` field with the current image id to the selected feature from the current image UI

tordans commented Apr 8, 2024 •

edited

Loading

tordans commented Apr 15, 2024 •

edited

Loading