-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Ctrl + Enter to submit forms #2540
Conversation
Signed-off-by: Jonas Franz <info@jonasfranz.software>
Signed-off-by: Jonas Franz <info@jonasfranz.software>
Codecov Report
@@ Coverage Diff @@
## master #2540 +/- ##
=======================================
Coverage 27.32% 27.32%
=======================================
Files 86 86
Lines 17135 17135
=======================================
Hits 4682 4682
Misses 11775 11775
Partials 678 678 Continue to review full report at Codecov.
|
public/js/index.js
Outdated
@@ -1786,6 +1787,14 @@ function initVueComponents(){ | |||
}) | |||
} | |||
|
|||
function initCtrlEnterSubmit() { | |||
$("textarea").keydown(function(e) { |
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.
I don't like this global/general event. What about creating class for that and use it in tmpl where you want to apply this feature?
public/js/index.js
Outdated
@@ -1786,6 +1787,14 @@ function initVueComponents(){ | |||
}) | |||
} | |||
|
|||
function initCtrlEnterSubmit() { | |||
$("textarea").keydown(function(e) { | |||
if ((e.ctrlKey || e.metaKey) && (e.keyCode == 13 || e.keyCode == 10)) { |
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.
This will not work on windows correctly as e.ctrlKey will fire also on AltGr+Enter. It should be something like:
(e.ctrlKey && !e.altKey) || e.metaKey
to check if ctrl/meta key is pressed
Checking if alt key is pressed too Signed-off-by: Jonas Franz <info@jonasfranz.software>
LGTM |
public/js/index.js
Outdated
@@ -1786,6 +1787,14 @@ function initVueComponents(){ | |||
}) | |||
} | |||
|
|||
function initCtrlEnterSubmit() { | |||
$(".ctrlenter").keydown(function(e) { |
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.
I was searching for better solution than using class. We can use custom attribute. e.g. "submit" (or "keysubmit", "autosubmit", 'submit="ctrl+enter"', ...) and do simple jq select like $("textarea[autosubmit]")
or $("[autosubmit]")
or $["[submit='ctrl+enter']"]
.
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.
@lafriks What do you think?
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.
why not use the custom data-
attributes? https://developer.mozilla.org/en-US/docs/Learn/HTML/Howto/Use_data_attributes
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.
I am not (pure/jq) js expert, but is not purpose of data-
attributes for storing data? We need only element selector. Using data-
would work too, but it has other meaning and purpose for me.
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.
E.g. boostrap uses data-
attributes for their javascript handlers intensively
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.
@Morlinest @daviian So can we agree on solution so that either @JonasFranzDEV can fix this or we can merge it as is
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.
@lafriks For me it is ok as it is. The class could be a little more descriptive, at first glance nobody would know that it is for form submission. Something like can-ctrlenter-submit
But I don't want to enforce my opinion.
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.
Or just take inspiration from github and use "js-quick-submit" class name.
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.
@JonasFranzDEV please change class name to @Morlinest suggested
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.
@lafriks Done
We should add Ctrl+Enter functionality to the description field of the new issue page as well. Also, the commit form in the web editor would also be nice. |
@ethantkoenig It is usable at the new issue page as well since it uses comment_tab as textarea. |
@JonasFranzDEV We should just make the comment editor a Vue Component |
LGTM |
Signed-off-by: Jonas Franz <info@jonasfranz.software>
This adds the functionality of submiting forms when Ctrl + Enter is pressed inside a textarea.
Fixes #2526