-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Add an option in chat preferences to disable enter key sends #6279
Conversation
652c605
to
3d735ac
Compare
@@ -55,6 +55,8 @@ | |||
align-items: center; | |||
background: none; | |||
border: none; | |||
width: 32px; |
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.
@ArgirisKatsoulieris This was a fix to make the send button line up with the other icons, all of which are fixed at 32px. It was due to the fact that this PR had not been merged yet. That PR has now been merged, so I believe this is an approved solution.
const bindingVar: any = { | ||
onShortKeyEnter: { | ||
key: 13, // 13 = Enter | ||
shortKey: true, | ||
handler: () => callbacksRef.current.onShortKeyEnter(), | ||
}, | ||
onEscape: { | ||
key: 27, | ||
handler: () => callbacksRef.current.onEscape(), | ||
}, // 27 = Escape | ||
onBackspace: { | ||
key: 8, | ||
handler: () => callbacksRef.current.onBackspace(), | ||
}, // 8 = Backspace | ||
}; |
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.
Moving it outside memo will make a performance a bit worst. Also can you use use a proper type instead of any?
onEnter: { | ||
key: 13, | ||
handler: () => callbacksRef.current.onEnter(), | ||
}, // 13 = 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.
You could do this
const canSubmitOnEnter = window.storage.get('enter-key-sends', true)
...
onEnter: canSubmitOnEnter ? {
key: 13,
handler: () => callbacksRef.current.onEnter(),
} : undefined,
@@ -452,7 +452,7 @@ export function CompositionArea({ | |||
|
|||
const sendButtonFragment = ( | |||
<> | |||
<div className="CompositionArea__placeholder" /> | |||
<div hidden={!large} className="CompositionArea__placeholder" /> |
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.
It is a good practice to solve one problem per PR ( because if something goes wrong it can be reverted very easily ). Can you make another PR just for that?
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.
Yes, I will split this into another PR
@@ -750,6 +750,7 @@ export function CompositionArea({ | |||
{stickerButtonFragment} | |||
{!dirty ? micButtonFragment : null} | |||
{attButton} | |||
{dirty || !shouldShowMicrophone ? sendButtonFragment : null} |
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.
It is a good practice to solve one problem per PR ( because if something goes wrong it can be reverted very easily ). Can you make another PR just for that?
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.
Yes, I will split this into another PR
For the UI: An other UX way to go, would be that the new setting is to "Keep composer mode enabled" or "Composer mode always enabled" which would have the same effect. |
I need this, and many more do: https://community.signalusers.org/t/options-to-change-behavior-of-pressing-the-enter-key/8753 |
I sincerely apologize, but I do not have the time to revisit this and resolve all the merge conflicts. If someone else wants to utilize the code and open a PR with the fixes you are welcome to, but I am going to close this PR since I will not be able to complete it at this time. |
First time contributor checklist:
Contributor checklist:
main
branchyarn ready
run passes successfully (more about tests here)Description
Changes Overview
These changes address the feature request made here. My changes include/will include:
Testing Approach