New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixed issue #651 - Keyboard Shortcuts #899

Merged
merged 1 commit into from May 7, 2015

Conversation

Projects
None yet
2 participants
@samranjbari

samranjbari commented Apr 28, 2015

  • Modified the Glimpse.axd to allow for disabling the shortcut keys. The checkbox is placed under more details link
  • Implemented the keys below
    var shortcutKeys = {
    Maximize: '103', // G
    Minimize: '103', // G (toggle)
    Popup: '112', // P
    Help: '63', // ?
    Close: '120', // X
    }
@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn May 6, 2015

Member

@samranjbari This is looking great! Sorry for the delay in getting back to this, got a new baby and been taking time out. Just wondering what browsers you have tested on? Also I'm afraid to say that the changes for the glimpse.js need to go over here - https://github.com/glimpse/glimpse.client/. Hopefully this will be a bit easier to work with. To build the client, assuming you have node installed, you just need to run node make. Really great effort and looking forward to getting this in!

Member

avanderhoorn commented May 6, 2015

@samranjbari This is looking great! Sorry for the delay in getting back to this, got a new baby and been taking time out. Just wondering what browsers you have tested on? Also I'm afraid to say that the changes for the glimpse.js need to go over here - https://github.com/glimpse/glimpse.client/. Hopefully this will be a bit easier to work with. To build the client, assuming you have node installed, you just need to run node make. Really great effort and looking forward to getting this in!

@avanderhoorn avanderhoorn added this to the vNext milestone May 6, 2015

@avanderhoorn avanderhoorn self-assigned this May 6, 2015

@samranjbari

This comment has been minimized.

Show comment
Hide comment
@samranjbari

samranjbari May 6, 2015

@avanderhoorn Congrats on the baby!! I tested on IE8>, chrome and FF all windows through browserstack. I can do more testing if needed.
I'll look into moving the changes.

samranjbari commented May 6, 2015

@avanderhoorn Congrats on the baby!! I tested on IE8>, chrome and FF all windows through browserstack. I can do more testing if needed.
I'll look into moving the changes.

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn May 6, 2015

Member

Thanks for letting me know about the tests. Sounds like more than we need ;) and appreciate you getting the change back into the client. I've gotten the release ready to go otherwise (5630c5e). After that, all that is left is if you could sign the CLA and send it to me at anthony.vanderhoorn at the google place.

Member

avanderhoorn commented May 6, 2015

Thanks for letting me know about the tests. Sounds like more than we need ;) and appreciate you getting the change back into the client. I've gotten the release ready to go otherwise (5630c5e). After that, all that is left is if you could sign the CLA and send it to me at anthony.vanderhoorn at the google place.

@samranjbari

This comment has been minimized.

Show comment
Hide comment
@samranjbari

samranjbari May 6, 2015

@avanderhoorn moved the changes to Glimpse.Client and requested a pulled.

samranjbari commented May 6, 2015

@avanderhoorn moved the changes to Glimpse.Client and requested a pulled.

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn May 7, 2015

Member

Great stuff!!! As soon as I get the CLA I'll get this in and the release out... currently working on the release notes, etc.

Member

avanderhoorn commented May 7, 2015

Great stuff!!! As soon as I get the CLA I'll get this in and the release out... currently working on the release notes, etc.

avanderhoorn added a commit that referenced this pull request May 7, 2015

@avanderhoorn avanderhoorn merged commit ecbf819 into Glimpse:master May 7, 2015

1 check passed

default Finished TeamCity Build Glimpse :: Continuous Integration : Tests passed: 942, ignored: 10
Details

@samranjbari samranjbari deleted the samranjbari:shortcutkeys branch May 7, 2015

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn May 7, 2015

Member

Found an issue in testing. When you are focused in a input and type any of the targeted keys, they come into effect. This means that when typing something like "gaggle of geese?", all sorts of things start to happen. This means we probably need to make it a combination key or know when any input is focused.

Member

avanderhoorn commented May 7, 2015

Found an issue in testing. When you are focused in a input and type any of the targeted keys, they come into effect. This means that when typing something like "gaggle of geese?", all sorts of things start to happen. This means we probably need to make it a combination key or know when any input is focused.

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn May 7, 2015

Member

We probably need to use a method like this - http://jsfiddle.net/bM6uY/4/:

$(document).bind('keypress', function(event) {
    if( event.which === 65 && event.shiftKey ) {
        alert('you pressed SHIFT+A');
    }
});
Member

avanderhoorn commented May 7, 2015

We probably need to use a method like this - http://jsfiddle.net/bM6uY/4/:

$(document).bind('keypress', function(event) {
    if( event.which === 65 && event.shiftKey ) {
        alert('you pressed SHIFT+A');
    }
});
@samranjbari

This comment has been minimized.

Show comment
Hide comment
@samranjbari

samranjbari May 7, 2015

I thought that was the purpose of the disable shortcut keys option. So they can disable the option in case of form controls.
What if they need to type upper case G in their text box? wouldn't this snippet still run?

samranjbari commented May 7, 2015

I thought that was the purpose of the disable shortcut keys option. So they can disable the option in case of form controls.
What if they need to type upper case G in their text box? wouldn't this snippet still run?

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn May 7, 2015

Member

With the exact sample I provided yes :) but we could use cntr or alt? Using either of these means that it would only be a problem if they had something that used those exact control combination... they shouldn't come across that problem in normal typing.

Member

avanderhoorn commented May 7, 2015

With the exact sample I provided yes :) but we could use cntr or alt? Using either of these means that it would only be a problem if they had something that used those exact control combination... they shouldn't come across that problem in normal typing.

@samranjbari

This comment has been minimized.

Show comment
Hide comment
@samranjbari

samranjbari May 7, 2015

ok, I will update the key mapping

samranjbari commented May 7, 2015

ok, I will update the key mapping

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn May 7, 2015

Member

Thanks!

Member

avanderhoorn commented May 7, 2015

Thanks!

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Jun 12, 2015

Member

Just wondering if you have had any luck with this? Its the last thing we are waiting on before being able to release.

Member

avanderhoorn commented Jun 12, 2015

Just wondering if you have had any luck with this? Its the last thing we are waiting on before being able to release.

@samranjbari

This comment has been minimized.

Show comment
Hide comment
@samranjbari

samranjbari Jun 12, 2015

I'll get on it this weekend. Thanks for checking in

samranjbari commented Jun 12, 2015

I'll get on it this weekend. Thanks for checking in

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Jun 12, 2015

Member

Really appreciate it!

Member

avanderhoorn commented Jun 12, 2015

Really appreciate it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment