-
Notifications
You must be signed in to change notification settings - Fork 164
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
Update jv-keyboard.js #789
Conversation
@bennylin Thanks for this patch! We will review it shortly. In the meantime, could you also create a task on Phabricator around this explaining in a few lines what is the issue this pull request is trying to address? You can then add |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the new characters you added, you might want to expand the test cases here:
jquery.ime/test/jquery.ime.test.fixtures.js
Line 2860 in 596391d
inputmethod: 'jv-keyboard', |
Is this a new procedure? Why two places and not centralized here? I have many PR that are waiting for review since last year while collecting dust. Do you want me to create ticket for each of them?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amire80 I have tested this PR and these changes look good to me. Could you take another look and merge it?
Add common shortcuts, fix bug, change url link
Add new tests
change the order
f7e4b77
to
f7d1b3f
Compare
@bennylin Thanks for creating the task! I requested that it be added to Phabricator so it can be prioritized more effectively. Apologies that your other pull requests haven't received attention. We hope to improve in this area! |
Thanks @srish ! |
Add common shortcuts, fix bug, change url link
Please merge, because the bug is quite severe, I've asked the bugfix since last year #760 760