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?

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 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

Open

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

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

904c802

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

tyrasd left a comment

laigyu commented Jan 24, 2024

tordans commented Apr 5, 2024

tordans commented Apr 5, 2024 •

edited

tordans commented Apr 5, 2024 •

edited

tordans commented Apr 5, 2024 •

edited

laigyu commented Apr 5, 2024

tordans commented Apr 6, 2024

laigyu commented Apr 7, 2024

tordans commented Apr 8, 2024 •

edited

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

laigyu commented Apr 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

Are you sure you want to change the base?

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

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

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

UseCase B: Show current mapillary image from the sidebar

tordans commented Apr 5, 2024 • edited

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

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

laigyu commented Apr 16, 2024

laigyu commented Dec 20, 2023 •

edited

tordans commented Apr 5, 2024 •

edited

tordans commented Apr 5, 2024 •

edited

tordans commented Apr 5, 2024 •

edited

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

tordans commented Apr 15, 2024 •

edited