Refactor how actions are specified and detected. #603
Conversation
This is primarily to make the code reusable in other modules. This results in some improvements: * Modifier keys can be ignored. For instance, ctrl-left drag defaults to rotation, shift must not be pressed, but the state of alt and meta is irrelevant. * You could add an action that requires two mouse buttons. This is a breaking change.
| * shift, ctrl, alt, meta | ||
| */ | ||
| actions: [{ | ||
| action: 'pan', |
There was a problem hiding this comment.
@manthey should we define action and event values as some keys at one single place? for e.g. (or some other way)
user_actions = {
pan: "pan",
zoom: "zoom"
}
and then we can have:
action: user_actions.pan
Thoughts?
|
Mostly looks good to me, has few suggestions. |
Conflicts: src/mapInteractor.js
Current coverage is 81.80% (diff: 98.31%)@@ master #603 diff @@
==========================================
Files 82 83 +1
Lines 7492 7563 +71
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 6121 6187 +66
- Misses 1371 1376 +5
Partials 0 0
|
|
@manthey are you planning to update this branch with suggestions as we discussed? |
|
@aashish24 Yes, soon. |
Trigger more events to allow other interactors more fine-grained control. For actions with a selection rectangle, use a flag in the action definition rather than a test against the action name. Return the action record from the eventMatch function. Add convenience functions for adding and removing actions.
|
thanks @manthey it's looking very good, will post my final comments by tomorrow morning. |
| * @property {object} event The triggering event | ||
| */ | ||
| ////////////////////////////////////////////////////////////////////////////// | ||
| geo_event.actiondown = 'geo_actiondown'; |
There was a problem hiding this comment.
@manthey nitpick: we use camelcase style for other event for example
geo_event.worldChanged = 'geo_worldChanged';
Should we change geo_event.actiondown to geo_event.actionDown? (and similarly for others too?)
There was a problem hiding this comment.
We haven't been consistent: geo_event.brushend = geo_brushend, for instance. I can change the actions to camel case or not as you prefer. I'd be tempted to make the events consistent in casing, but that would be a breaking change for not much benefit.
There was a problem hiding this comment.
We haven't been consistent: geo_event.brushend = geo_brushend, for instance. I can change the actions to camel case or not as you prefer. I'd be tempted to make the events consistent in casing, but that would be a breaking change for not much benefit.
I think it would be nice to have the consistency IMO but we can file the bug for now and break it when we release 1.0. I would be okay with that. Thoughts?
The term 'event' was overused.
|
other than my new comments, the branch LGTM 👍 |
As part of this, there doesn't need to be a one-to-one correspondence between actions and events. Moreover, selection rectangle-based actions can trigger any event.
|
@aashish24 I've added actions as a set of defined constants and updated the tests. |
| rotate: 'geo_action_rotate', | ||
| select: 'geo_action_select', | ||
| unzoomselect: 'geo_action_unzoomselect', | ||
| zoom: 'zoom', |
There was a problem hiding this comment.
I believe this is intentional (meaning not "geo_zoom")?
|
Looking much better @manthey just had one question on "zoom" |
|
@aashish24 Updated. |
|
LGTM 👍 thanks |
This is primarily to make the code reusable in other modules. This results in some improvements:
This is a breaking change.