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.

Sign up for GitHub

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

Jump to bottom

Add an option in chat preferences to disable enter key sends #6279

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Add an option in chat preferences to disable enter key sends #6279

wants to merge 6 commits into from

Conversation

hkup859
Copy link
Contributor

@hkup859 hkup859 commented Feb 7, 2023 •
edited

First time contributor checklist:

Contributor checklist:

  • My contribution is not related to translations.
  • My commits are in nice logical chunks with good commit messages
  • My changes are rebased on the latest main branch
  • A yarn ready run passes successfully (more about tests here)
  • My changes are ready to be shipped to users

Description

Changes Overview

These changes address the feature request made here. My changes include/will include:

  • The send button will be visible when text has been entered. This models the behavior after the android app so it feels the same. An alternative approach would be to always have it showing, which I can do if that is preferred.
  • There will be a new option in the Preferences -> Chats section that is the same as the android app: "Enter key sends". To avoid confusing any existing users this option will be ON by default to replicate the existing behavior. Turning it off will cause the only way to send a message to be via Ctrl/Command + Enter or with the Send button that will now be visible. If there are other ways to send a message let me know and I can test to ensure I don't break them.
  • Fix the send button margin issue using the requested approach in Fix margin on the send button during large composition mode #6219.

Testing Approach

  • I am manually testing sending messages using the collapsed view, expanded view, and with both the new option on and off.
  • I have not written any new tests as I could not find a similar example test to base it off of. If someone can point out a test that either is based on a settings value (for testing the new preference value) or a test for testing that keypresses function properly I am happy to implement some tests.
  • I have tested on MacOS (Ventura 13.0.1)

@hkup859 hkup859 changed the title Add send button when text is present Add an option in chat preferences to disable enter key sends Feb 8, 2023
@hkup859 hkup859 marked this pull request as ready for review February 14, 2023 22:06
@hkup859 hkup859 closed this Feb 16, 2023
@hkup859 hkup859 deleted the allow-optional-send-on-enter branch February 16, 2023 15:23
@hkup859 hkup859 restored the allow-optional-send-on-enter branch February 16, 2023 15:23
@hkup859 hkup859 reopened this Feb 16, 2023
@indutny-signal indutny-signal self-assigned this Feb 16, 2023
hkup859
hkup859 commented Mar 24, 2023
View reviewed changes
stylesheets/components/CompositionArea.scss Outdated
@@ -55,6 +55,8 @@
align-items: center;
background: none;
border: none;
width: 32px;
Copy link
Contributor Author

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.

sztadii
sztadii reviewed Jul 17, 2023
View reviewed changes
ts/components/CompositionInput.tsx
Comment on lines +566 to +580
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
};
Copy link

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?

sztadii
sztadii reviewed Jul 17, 2023
View reviewed changes
ts/components/CompositionInput.tsx
Comment on lines -588 to -710
onEnter: {
key: 13,
handler: () => callbacksRef.current.onEnter(),
}, // 13 = Enter
Copy link

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,

sztadii
sztadii reviewed Jul 17, 2023
View reviewed changes
ts/components/CompositionArea.tsx
@@ -452,7 +452,7 @@ export function CompositionArea({

const sendButtonFragment = (
<>
<div className="CompositionArea__placeholder" />
<div hidden={!large} className="CompositionArea__placeholder" />
Copy link

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?

Copy link
Contributor Author

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

sztadii
sztadii reviewed Jul 17, 2023
View reviewed changes
ts/components/CompositionArea.tsx
@@ -750,6 +750,7 @@ export function CompositionArea({
{stickerButtonFragment}
{!dirty ? micButtonFragment : null}
{attButton}
{dirty || !shouldShowMicrophone ? sendButtonFragment : null}
Copy link

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?

Copy link
Contributor Author

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

@1u
Copy link

1u commented Jan 15, 2024

For the UI:
Doesn't it need an explanation in the settings that "Ctrl-Enter" will send the message when enabled?

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.

@1u
Copy link

1u commented Apr 20, 2024

I need this, and many more do: https://community.signalusers.org/t/options-to-change-behavior-of-pressing-the-enter-key/8753

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

@sztadii sztadii sztadii left review comments

Assignees

@indutny-signal indutny-signal

Labels
PR: Needs Review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@hkup859 @1u @sztadii @indutny-signal