Skip to content
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

[feat]: New Shortcuts #1044

Merged
merged 14 commits into from
May 5, 2020
Merged

Conversation

AkalUstat
Copy link
Collaborator

@AkalUstat AkalUstat commented Apr 27, 2020

https://drive.google.com/open?id=1ILu1IqxxnFCV8Rs4BAHHC0qTl3CvTdx_6p-EeA--lKY

  • Passed sanity tests.
  • Ran npm test & fixed newly introduced lint errors.
  • Checked console for errors.

center align, vishraams, larivar
@AkalUstat AkalUstat linked an issue Apr 27, 2020 that may be closed by this pull request
@AkalUstat AkalUstat marked this pull request as ready for review April 27, 2020 20:29
@Gauravjeetsingh Gauravjeetsingh temporarily deployed to khalis-herok-shortcuts-jwps3u1 April 28, 2020 14:42 Inactive
@Gauravjeetsingh
Copy link
Collaborator

@AkalUstat ji, I am unable to use the keyboard shortcuts.

Testing it here
https://khalis-herok-shortcuts-jwps3u1.herokuapp.com/shabad?id=903

Do I need to enable it from somewhere or something?

@AkalUstat
Copy link
Collaborator Author

AkalUstat commented Apr 28, 2020

You have to be clicked on the shabad viewer. Found that hukamnama, search, and ang have that component in common so that is where i put. I could put it higher up in the component tree if you like

@Gauravjeetsingh
Copy link
Collaborator

Can you check once, it's not working for me in hukamnama page as well. Also, I see the blue outline everywhere.
image

@Gauravjeetsingh Gauravjeetsingh temporarily deployed to khalis-herok-shortcuts-jwps3u1 April 28, 2020 14:56 Inactive
Copy link
Collaborator

@Gauravjeetsingh Gauravjeetsingh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keys not working as of now (which you have mostly fixed I think), and there's a blue outline on the viewer.

and errors with alt+e
@Gauravjeetsingh Gauravjeetsingh temporarily deployed to khalis-herok-shortcuts-jwps3u1 April 28, 2020 15:26 Inactive
@Gauravjeetsingh Gauravjeetsingh temporarily deployed to khalis-herok-shortcuts-jwps3u1 April 28, 2020 15:30 Inactive
@Gauravjeetsingh Gauravjeetsingh temporarily deployed to khalis-herok-shortcuts-jwps3u1 April 28, 2020 15:56 Inactive
@Gauravjeetsingh Gauravjeetsingh had a problem deploying to khalis-herok-shortcuts-jwps3u1 April 28, 2020 16:01 Failure
@AkalUstat
Copy link
Collaborator Author

@saintsoldierx I will create some user info for this stuff in a separate PR

@Gauravjeetsingh Gauravjeetsingh temporarily deployed to khalis-herok-shortcuts-jwps3u1 April 28, 2020 17:07 Inactive
Copy link
Collaborator

@Gauravjeetsingh Gauravjeetsingh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Awesome work @AkalUstat

I only have a concern about the keyboard shortcuts we use
For Shahmukhi transliteration toggle, we use: ctrl + alt + shift + t + s, that's five keys. It takes both hands to toggle that. It can be done with one hand as well, but all fingers are used and are put in a weird position.

Also, @AkalUstat are we using the same shortcut keys that are currently in desktop, if not, we should.

@saintsoldierx @maneetpaul @ManjotS veerjis, can any of you you review the shortcut keys used here if you haven't yet.

@AkalUstat
Copy link
Collaborator Author

Looks good to me. Awesome work @AkalUstat

I only have a concern about the keyboard shortcuts we use
For Shahmukhi translation toggle, we use: ctrl + alt + shift + t + s, that's five keys. It takes both hands to toggle that. It can be done with one hand as well, but all fingers are used and are put in a weird position.

Also, @AkalUstat are we using the same shortcut keys that are currently in desktop, if not, we should.

@saintsoldierx @maneetpaul @ManjotS veerjis, can any of you you review the shortcut keys used here if you haven't yet.

  1. Yea I agree with the translit toggle; problem is if we do cmd+alt+e (for english translit, without the shift and the t) due to an issue with alt +_ e it messes up all the other shortcuts. I can try to debug and see if i can come up with resolution

  2. Only desktop one that matches is Cmd + /. Desktop does not have shortcuts to modify display items. Waiting on react refactor to add these to desktop

@Gauravjeetsingh
Copy link
Collaborator

Found a small bug
For toggling Shahmukhi transliteration, one has to take the hand up from all five keys and press again.
For example with Hindi transliteration, I just pick my finger from last key and press again and it works.

@AkalUstat
Copy link
Collaborator Author

Interesting...I will try to debug as well

@AkalUstat AkalUstat closed this Apr 29, 2020
@AkalUstat AkalUstat reopened this Apr 29, 2020
@AkalUstat
Copy link
Collaborator Author

There is error when the alt key is used for shortcuts in the library, documented here greena13/react-hotkeys#269 so we need new mappings for translit characters to avoid the long shortcuts

}

// export default Shortcuts;
export {ViewerShortcutHanders, ViewerShortcuts, GlobalHandlers, GlobalShortcuts}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. We should also document the keyboard shortcuts both in the ReadMe.md and somewhere in the application - may be in the footer? @saintsoldierx thoughts?

Copy link
Collaborator Author

@AkalUstat AkalUstat Apr 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking maybe the help menu (but for another pr) and then a quick help toggle at the top bar

@AkalUstat
Copy link
Collaborator Author

AkalUstat commented Apr 30, 2020

New transliteration keymaps:
shift+e: english
shift+s: shahmukhi
shift+h hindi

and transliteration shift + key
@Gauravjeetsingh Gauravjeetsingh temporarily deployed to khalis-herok-shortcuts-wlhjduh May 4, 2020 04:20 Inactive
Copy link
Collaborator

@Gauravjeetsingh Gauravjeetsingh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. ✔️
Just a suggestion for one more shortcut; can we do shift + v for Larivaar assist?

@AkalUstat
Copy link
Collaborator Author

added shift + l shortcut for larivar assist

@Gauravjeetsingh Gauravjeetsingh temporarily deployed to khalis-herok-shortcuts-wlhjduh May 4, 2020 16:39 Inactive
@saintsoldierx saintsoldierx merged commit f58a5a1 into KhalisFoundation:dev May 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add keyboard shortcuts
4 participants