Optional expose on right click #78
Conversation
* Adds `wmOptions` to constructor of WindowManager. * Adds `exposeOnRightClick` to `wmOptions` (default: true). * Adds function to add the value to `expose.js`, so the register method knows the value. * Updates `README.md`. Signed-off-by: Daniel Ravenshorst <daniel.ravenshorst@dealerdirect.nl>
* Updates `README.md`. Signed-off-by: Daniel Ravenshorst <daniel.ravenshorst@dealerdirect.nl>
Signed-off-by: Daniel Ravenshorst <daniel.ravenshorst@dealerdirect.nl> # Conflicts: # dist/ventus.min.js
rlamana
left a comment
rlamana
left a comment
There was a problem hiding this comment.
Thanks a lot for all these improvements!
README.md
Outdated
| Default is true (right click shows expose). | ||
|
|
||
| var wm = new Ventus.WindowManager({ | ||
| exposeOnRightClick: true |
There was a problem hiding this comment.
I was just going to suggest to change the previous name to something like this. Maybe something shorther? exposeOnMouse? or maybe use a string to specify a trigger, something like exposeShortcut = 'rightclick' | 'a' | 'enter'...
src/ventus/wm/modes/expose.js
Outdated
| if(self.windows.length > 0) { | ||
| self.mode = 'expose'; | ||
| if (exposeOnRightClick === true) { | ||
| this.el.on('contextmenu', _.throttle(function() { |
There was a problem hiding this comment.
If the goal of setExposeOnRightClick is to be able to dynamically enable/disable this option, the registration of the event listener should happen there.
However, to avoid register/unregister the listener every time the option changes, we could just check the value of exposeOnRightClick inside the listener code.
src/ventus/wm/modes/expose.js
Outdated
|
|
||
| var ExposeMode = { | ||
| // Enable/disable expose on right-click (true=disable / false=enable) | ||
| setExposeOnRightClick: function (value) { |
There was a problem hiding this comment.
To keep consistency with other parts of the code, we could use a setter for this.
* Removes `setExposeOnRightClick` as it's not needed. * Adds jQuery `$` to expose.js * Adds validListeners, like a,enter,space,rightclick. * Adds the exposeListeners in WindowManager. * Updates README.md * Updates the `dist` folder. * Updates the examples. Signed-off-by: Daniel Ravenshorst <daniel.ravenshorst@dealerdirect.nl>
|
@rlamana I have fixed your concerns. Available listeners are If you don't specify anything it will use the default Can you look at it again. Thanks |
src/ventus/wm/windowmanager.js
Outdated
| 'use strict'; | ||
|
|
||
| var WindowManager = function () { | ||
| var WindowManager = function (wmOptions) { |
There was a problem hiding this comment.
nit: remove wm- prefix. In the context of the WindowManager "class" this is redundant.
src/ventus/wm/modes/expose.js
Outdated
| }; | ||
|
|
||
| var validListeners = { | ||
| a: keypressCallback(97), |
There was a problem hiding this comment.
also not very important, but technically that function is not a callback nor it returns a callback exactly. I would rename it to something more descriptive like configKeyListener.
* Merges master into current branch. * Renames function `keypressCallback` to `configKeyListener` in `expose.js`. * Renames `wmOptions` to `options` in `windowmanager.js`. Signed-off-by: Daniel Ravenshorst <daniel.ravenshorst@dealerdirect.nl>
* Renames function `keypressCallback` to `configKeyListener` in `expose.js`. * Renames `wmOptions` to `options` in `windowmanager.js`. Signed-off-by: Daniel Ravenshorst <daniel.ravenshorst@dealerdirect.nl>
|
@rlamana I have fixed your concerns, can you have look at it again? |
|
Please resolve the conflicts. These problems with the dist files should be solved once the branch v0.3.0 is merged. This is a branch where I have been working on a migration to webpack and upgraded a little this old code. |
…nal-expose-on-right-click Signed-off-by: Daniel Ravenshorst <daniel.ravenshorst@dealerdirect.nl> # Conflicts: # dist/ventus.min.js
…nal-expose-on-right-click Signed-off-by: Daniel Ravenshorst <daniel.ravenshorst@dealerdirect.nl> # Conflicts: # dist/ventus.min.js
|
@Dravenshorst, please solve the conflicts. Appreciate if this PR gets merged. |
3 participants
Adds the option to disable/enable expose on right click.