Replace radial menu with a context menu #3753

kepta · 2017-01-13T05:55:53Z

Todo

Fix the tooltip
Remove the redundant variables for adjusting the position of svgs in edit_menu.js
Fix the failing test cases

(closes #3671 )

bhousel · 2017-01-18T13:40:36Z

Hey @samanpwbb can you check the screenshot here #3671 (comment)
and let us know if you have any thoughts?

samanpwbb · 2017-01-19T00:50:52Z

I think the whole form can be vertically condensed, and simplified. I would lose the semitransparent background – it's distracting and less legible than a solid background. If you make the background solid, you will be able to remove the round fills from under the action icons, which will allow you to cut a good amount of vertical space from the design. End result will be cleaner looking and easier to use.

kepta · 2017-02-07T17:49:23Z

@bhousel I feel this is done.
This test iD.behaviorSelect "before each" hook for "refuse to enter select mode with no ids": randomly fails on travis. :/

Could you review it ?
I have kept the radial_menu.js for semver reasons, ( related question: what should I do with the css styles named radial_menu, currently edit_menu.js uses them ? )
I hope I am not missing any test cases like I did for the nominatim patch.

kepta · 2017-02-07T19:21:57Z

I tried a white style. Notice the slimmer height of each operation and a disable cursor.

cc: @samanpwbb @rasagy

kepta · 2017-02-08T07:10:38Z

As per talk with @rasagy, he suggested on removing the operations which are always disabled.
This would help reduce the height of context menu.
We took a bunch of screenshots.

In this example, the button would always be disabled.

vvv These examples need to show the operation.

bhousel · 2017-02-08T14:08:37Z

As per talk with @rasagy, he suggested on removing the operations which are always disabled.
This would help reduce the height of context menu.
We took a bunch of screenshots.

I know that the disabled icons were always there for discoverability. I still think messages like "This feature can't be made circular because it's not a loop" are important for users to see.

Maybe we can just make the icons smaller? This menu doesn't need to be so much bigger than the browser's default contextmenu.

I really like the light style! 👍

slhh · 2017-02-09T02:17:38Z

@bhousel
Discoverability is important, but we don't need to overdo it. Always showing every operation seems to be too much, but this doesn't mean that we should always hide disabled operations.
We should always think about what is really required to make the specific operation sufficiently discoverable.

For example "Circularize":
Someone who is not working with nearly circular features doesn't need "Circularize".
Someone who is working with such will eventually select such a feature and discover the "Circularize" operation, even if it is shown in case of nearly circular closed features only.

I still think messages like "This feature can't be made circular because it's not a loop" are important for users to see.

Showing this message can be limited to the case of a nearly closed, nearly circular way.

Maybe we can just make the icons smaller? This menu doesn't need to be so much bigger than the browser's default contextmenu.

Doesn't this make the icons too unspecific? Otherwise I would agree.
Maybe we can reduce the height of the menu rows, but place the icons aligned in two colums alternatingly (zigzag pattern).

Fixed - On reselecting multiple entities, right click doesn’t discard selection - On selecting new entity, right click discards previous selection - Preserved shift selection for both left & right click

rasagy · 2017-02-09T16:05:17Z

I know that the disabled icons were always there for discoverability. I still think messages like "This feature can't be made circular because it's not a loop" are important for users to see.

@bhousel I think the idea was not to hide all the disabled icons, but only ones that didn’t seem relevant for a particular feature. On exploring a bit with @kepta, we realized that this already happens. Here’s what we felt: actions that might be disabled due to the feature not being in the screen should be shown, since they explain why something can’t be done, but actions that won’t be possible for a feature (straight road would never be able to Circularize), it only adds more confusion.

Do you think there are more cases — like Circularize for straight roads — that would be disabled regardless of the context? If this isn’t common, then maybe we can take a quick call on hiding Circularize when it’s not relevant.

bhousel · 2017-02-09T17:18:29Z

The iD architecture document mentions disabled operations a bit here: https://github.com/openstreetmap/iD/blob/master/ARCHITECTURE.md#operations-module

I don't know who the original author of this section was, but I think it's still important to show disabled operations for discoverability.

The menu is there for teaching in addition to using. So please keep Circularize on the menu, but disabled for straight roads.

bhousel · 2017-02-09T18:12:55Z

modules/behavior/select.js

@@ -23,6 +23,12 @@ export function behaviorSelect(context) {


    function click() {
+        var rtClick = d3.event.type === 'contextmenu';   


contextmenu is not quite right click. On a Mac it fires on mousedown, and on Windows it fires on mouseup.

The code here might be totally ok, this is just a reminder to myself to check the reselection behavior on Windows.

bhousel · 2017-02-09T18:15:16Z

modules/modes/select.js

        } else {
            var point = context.mouse(),
-                viewport = geoExtent(context.projection.clipExtent()).polygon();
+                viewport = geoExtent(context.projection.clipExtent()).polygon(),


I have to check, I think there is a context.map().trimmedExtent() that can be used to get a smaller viewport avoiding UI controls around the edges.

Yes, it totally makes sense to use context.map().trimmedExtent().
I also created this context.map().trimmedExtentPadding(p) for the lane work, which accepts additional padding, should we have such a thing?

I also created this context.map().trimmedExtentPadding(p) for the lane work, which accepts additional padding, should we have such a thing?

Instead of adding another method, can we just have trimmedExtent accept an optional argument with a padding rect?

bhousel · 2017-02-09T18:17:49Z

modules/renderer/map.js

@@ -311,7 +311,7 @@ export function rendererMap(context) {
    function resetTransform() {
        if (!transformed) return false;

-        surface.selectAll('.radial-menu').interrupt().remove();
+        surface.selectAll('.edit-menu').interrupt().remove();


If we're going to keep .radial-menu around for backward compatibility and fork projects, we need to include it in the selector here.

Well, so we will end up having two of these (one for radial, other for edit).
I am thinking if about all the cleaning that would need to be done for a major release ..

I think that is ok.. selectAll takes a CSS selector, so it can be written like:

surface.selectAll('.radial-menu, .edit-menu').interrupt().remove();

We can tell people that the radial menu is deprecated and will be removed in iD v3.

bhousel · 2017-02-09T18:21:30Z

modules/ui/intro/point.js

@@ -143,7 +143,7 @@ export function uiIntroPoint(context, reveal) {
            context.history().on('change.intro', deleted);

            setTimeout(function() {
-                var node = d3.select('.radial-menu-item-delete').node();
+                var node = d3.select('.edit-menu-item-delete').node();


Reminder to 1. keep radial-menu classes around for backward compat. 2. test what this actually does when we run through the tutorial.

bhousel · 2017-02-10T18:42:07Z

I edited a lot with this today to test it out. Overall it's great.
I really don't miss the menu except in one situation, and that's when operations are disabled.

One example of this is: I'd draw a building and hit S to square the corners of it, and nothing would happen because too much of the building is off screen. Normally you would be able to see the menu with the disabled icons and realize that something is preventing you from running the command. But without a menu, I'd find myself wondering whether the command ran or not, and just keep hitting S, then eventually right-click to see the menu and realize what was going on.

This is a hard problem to solve elegantly. #1734 touches on this - "Better indication when an action is selected". That issue is, sometimes a user will do something by accident and not realize that the action happened. But now without a menu, it's also possible for a user to try to do something on purpose and not realize when the action didn't happen.

I'm thinking for keyed operations initiated by the user, we should maybe take the message that we already use for undo/redo (e.g. "Made an area circular") and flash it in the corner of the screen. If the operation is disabled, instead flash the disabled message "This can't be made circular because it isn't a loop".

Flash messages are really hard to get right, in a way that is tasteful, enjoyable, and delights the user instead of being annoying. But it would close #1734 and help us replace the visual clues that we lose by not popping up the context menu on every select. I'm going to prototype this idea and see how it turns out.

kepta · 2017-02-10T18:56:39Z

@bhousel I like the idea of notifying the user that something is not allowed. In the Mac they play that famous sound when an operation is disabled.
I just want to make sure, by flash messages, we are not planning to blink or do something fancy to grab the users attention right?

I really like the way visual studio code shows alerts. They just overlay them on the top.

Maybe we could do something similar for iD? Showing the alert at the top (even if it hides something). Some of these alerts would go off and some wouldn't , depending on the severity.
This is also similar to how iOS shows notifications.

bhousel · 2017-02-17T05:28:17Z

Latest version.. I like it! Thoughts @samanpwbb @kepta ?

kepta · 2017-02-17T09:34:08Z

@bhousel, I would love to see a different colour on it. (😅 pink looks good to me)

runnabot · 2017-02-17T16:50:39Z

Deployed iD-runnable/edit_menu. View on Runnable.
_{From Runnable}

bhousel · 2017-02-17T19:20:40Z

Final stuff:

Show operation title above tooltip
Check contextmenu/right-click especially on Windows
Check contextmenu preventdefault ~~(right click seems to bring up menu but not activate frame?)~~
Update: I don't know how to fix this by simulating a click, not important now.
Preserve backward compatibility of radial menu
Update walkthrough
RTL

Old menu behavior can be restored with 2 cookies: - `edit-menu-style=radial` - Display menu as a radial menu, limited to 8 items - `edit-menu-show-always=1` - Show menu on all clicks, not just contextmenu/right

(closes #3029)

bhousel · 2017-02-25T20:33:30Z

This was merged! 🎉
Thanks @kepta @rasagy @samanpwbb & @slhh!

kepta · 2017-02-26T03:27:09Z

Awesome🤘

…

On Sun, 26 Feb 2017 at 2:03 AM, Bryan Housel ***@***.***> wrote: This was merged! 🎉 Thanks @kepta <https://github.com/kepta> @rasagy <https://github.com/rasagy> @samanpwbb <https://github.com/samanpwbb> & @slhh <https://github.com/slhh>! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3753 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGpL7sn8-DmxxoHxDxEhXma8yYr2vTbMks5rgJAcgaJpZM4LijMP> .

gmaclennan · 2017-02-26T03:31:31Z

Great work! Testing this in Chrome 56.0.2924.87 when I select an element and hit SPACE to bring up the edit menu, it appears in the top right of the screen, away from the selected element.

bhousel · 2017-02-26T03:37:32Z

Great work! Testing this in Chrome 56.0.2924.87 when I select an element and hit SPACE to bring up the edit menu, it appears in the top right of the screen, away from the selected element.

Good catch, I notice that too.. I'll open a new issue for it..

kepta added the wip Work in progress label Jan 13, 2017

kepta force-pushed the edit_menu branch from 6cf86b7 to bd1a70a Compare February 7, 2017 17:42

kepta removed the wip Work in progress label Feb 7, 2017

kepta force-pushed the edit_menu branch 2 times, most recently from c540e89 to df71690 Compare February 8, 2017 08:08

kepta added 4 commits February 9, 2017 12:53

Add context menu

5b92b90

white theme for edit_menu.js

11d7cc7

fix tooltip overflow

734f40c

semver compliance for edit_menu

ca25a34

kepta force-pushed the edit_menu branch 3 times, most recently from b8f4928 to 308917e Compare February 9, 2017 10:07

Fix right clicking edge cases

07de6d9

Fixed - On reselecting multiple entities, right click doesn’t discard selection - On selecting new entity, right click discards previous selection - Preserved shift selection for both left & right click

kepta force-pushed the edit_menu branch from 308917e to 07de6d9 Compare February 9, 2017 10:11

bhousel reviewed Feb 9, 2017

View reviewed changes

Merge branch 'master' into edit_menu

57aea40

bhousel mentioned this pull request Feb 17, 2017

Browse mode enhancement #3843

Closed

bhousel added 2 commits February 17, 2017 15:47

Add title to editmenu tooltip

9d66007

Variable names, fix edge avoidance, tooltips placement, RTL

d8237fa

bhousel force-pushed the edit_menu branch from c91388b to d8237fa Compare February 18, 2017 20:43

bhousel added 10 commits February 21, 2017 15:47

suppressMenu(true) is now the default

e756520

More improvements to select behavior re contextmenu, shiftclick, etc.

4f8d772

Preserve backward compability with radial menu

f7c3de9

Old menu behavior can be restored with 2 cookies: - `edit-menu-style=radial` - Display menu as a radial menu, limited to 8 items - `edit-menu-show-always=1` - Show menu on all clicks, not just contextmenu/right

Add mention of the right-click menu in the walkthrough

dc2318a

Fix RTL for flash messages

38f6d2d

Fix RTL for scale bar

6ef85c4

Remove ineffective attempt to grab focus

f291b0a

Normalize mousewheel zooming across browsers

334188c

(closes #3029)

Fix mouse xy calculation for mousewheel scroll normalization

5ba7229

Quote "-apple-system" to make IE11 happy

bf71756

bhousel merged commit bf71756 into master Feb 25, 2017

bhousel deleted the edit_menu branch February 25, 2017 20:33

bhousel removed the wip Work in progress label Feb 25, 2017

This was referenced Feb 26, 2017

Edit menu should be positioned by initial select click coordinate #3862

Closed

Right now it is impossible to select between multiple overlapping polygons #2225

Closed

bhousel mentioned this pull request Mar 8, 2017

Better indication when an action is selected #1734

Closed

althio mentioned this pull request May 24, 2017

Context menus disappeared #4065

Closed

quincylvania added a commit that referenced this pull request Jun 16, 2019

Remove deprecated radial menu (re: #3753)

b15bbee

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace radial menu with a context menu #3753

Replace radial menu with a context menu #3753

kepta commented Jan 13, 2017 •

edited

Loading

bhousel commented Jan 18, 2017

samanpwbb commented Jan 19, 2017 •

edited

Loading

kepta commented Feb 7, 2017 •

edited

Loading

kepta commented Feb 7, 2017 •

edited

Loading

kepta commented Feb 8, 2017 •

edited

Loading

bhousel commented Feb 8, 2017 •

edited

Loading

slhh commented Feb 9, 2017

rasagy commented Feb 9, 2017

bhousel commented Feb 9, 2017

bhousel Feb 9, 2017

bhousel Feb 9, 2017

kepta Feb 10, 2017 •

edited

Loading

bhousel Feb 10, 2017

bhousel Feb 9, 2017

kepta Feb 10, 2017

bhousel Feb 10, 2017

bhousel Feb 9, 2017

bhousel commented Feb 10, 2017 •

edited

Loading

kepta commented Feb 10, 2017 •

edited

Loading

bhousel commented Feb 17, 2017

kepta commented Feb 17, 2017

runnabot commented Feb 17, 2017

bhousel commented Feb 17, 2017 •

edited

Loading

bhousel commented Feb 25, 2017

kepta commented Feb 26, 2017 via email

gmaclennan commented Feb 26, 2017

bhousel commented Feb 26, 2017

		@@ -23,6 +23,12 @@ export function behaviorSelect(context) {


		function click() {
		var rtClick = d3.event.type === 'contextmenu';

Replace radial menu with a context menu #3753

Replace radial menu with a context menu #3753

Conversation

kepta commented Jan 13, 2017 • edited Loading

bhousel commented Jan 18, 2017

samanpwbb commented Jan 19, 2017 • edited Loading

kepta commented Feb 7, 2017 • edited Loading

kepta commented Feb 7, 2017 • edited Loading

kepta commented Feb 8, 2017 • edited Loading

bhousel commented Feb 8, 2017 • edited Loading

slhh commented Feb 9, 2017

rasagy commented Feb 9, 2017

bhousel commented Feb 9, 2017

bhousel Feb 9, 2017

Choose a reason for hiding this comment

bhousel Feb 9, 2017

Choose a reason for hiding this comment

kepta Feb 10, 2017 • edited Loading

Choose a reason for hiding this comment

bhousel Feb 10, 2017

Choose a reason for hiding this comment

bhousel Feb 9, 2017

Choose a reason for hiding this comment

kepta Feb 10, 2017

Choose a reason for hiding this comment

bhousel Feb 10, 2017

Choose a reason for hiding this comment

bhousel Feb 9, 2017

Choose a reason for hiding this comment

bhousel commented Feb 10, 2017 • edited Loading

kepta commented Feb 10, 2017 • edited Loading

bhousel commented Feb 17, 2017

kepta commented Feb 17, 2017

runnabot commented Feb 17, 2017

bhousel commented Feb 17, 2017 • edited Loading

bhousel commented Feb 25, 2017

kepta commented Feb 26, 2017 via email

gmaclennan commented Feb 26, 2017

bhousel commented Feb 26, 2017

kepta commented Jan 13, 2017 •

edited

Loading

samanpwbb commented Jan 19, 2017 •

edited

Loading

kepta commented Feb 7, 2017 •

edited

Loading

kepta commented Feb 7, 2017 •

edited

Loading

kepta commented Feb 8, 2017 •

edited

Loading

bhousel commented Feb 8, 2017 •

edited

Loading

kepta Feb 10, 2017 •

edited

Loading

bhousel commented Feb 10, 2017 •

edited

Loading

kepta commented Feb 10, 2017 •

edited

Loading

bhousel commented Feb 17, 2017 •

edited

Loading