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

Fixes #209: Allow inserting emojis alongside text #202

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

bLind17
Copy link

@bLind17 bLind17 commented Nov 11, 2020

For my current project I needed to insert emojis into the input field rather then send them as a message directly.
This PR includes changes to realize that.

I have added a property sendEmojisDirectly that propagates towards the UserInput.
If set to false the new feature - respecting the last selection that has been made in the input fileld - will either insert the emoji at the caret or overwrite a selected range.

@a-kriya
Copy link
Collaborator

a-kriya commented Dec 9, 2020

@bLind17 Thank you for the PR. The emoji picker updates are great and seem like that should be the default behavior.

  • First, would you mind splitting off changes from 60a6d94 into a separate PR? Those changes seem unrelated to what this PR is about as you've described in your post.
  • Second, since this is an improvement that brings the behavior closer to expected, I think there is no need for sendEmojisDirectly prop, and this can be the default behavior.
  • Lastly, I think the code can be simplified a lot if we compromise on the edge case of selecting an emoji while some text is selected in the message field. When the emoji selector is clicked, the text selection can simply be cleared (not exactly unexpected since the focused element changes) and any selected emoji would be placed at the caret.

@a-kriya a-kriya changed the title Option to use the EmojiPicker like a keyboard Allow inserting emojis alongside text Dec 9, 2020
@a-kriya a-kriya changed the title Allow inserting emojis alongside text Fixes #209: Allow inserting emojis alongside text Dec 9, 2020
@bLind17
Copy link
Author

bLind17 commented Dec 10, 2020

* First, would you mind splitting off changes from [60a6d94](https://github.com/mattmezza/vue-beautiful-chat/commit/60a6d94f27fc334ca0d15c8b2626e3b557304b1d) into a separate PR? Those changes seem unrelated to what this PR is about as you've described in your post.

I have removed that commit, since I completely rewrote the chat window for my project, and I'm not using this code anymore 😉

* Second, since this is an improvement that brings the behavior closer to expected, I think there is no need for `sendEmojisDirectly` prop, and this can be the default behavior.

I added that as a precaution, since I didn't know how you would deal with such major changes. I think it could be left out, but that would totally change the behaviour on existing projects, maybe there are some people out there who would be bothered by it?

* Lastly, I think the code can be simplified a lot if we compromise on the edge case of selecting an emoji while some text is selected in the message field. When the emoji selector is clicked, the text selection can simply be cleared (not exactly unexpected since the focused element changes) and any selected emoji would be placed at the caret.

Again, this is your choice, but I would never comprimise on usability to get cleaner code. If I select some text and then add an emoji, I expect the text to be overwritten, as that is the common behaviour.
I did add all those selection checks because before, I managed to replace the contents of the rest of the page when trying to insert an emoji... It may well be that those can be simplified, but as far as I know, my changes work properly. And since I have written my own version of the chat window and thus have abandoned my fork of your project, I'm not going to have the time to implement and test further solutions.

My new chat windows is based on your implementation and ideas, thank you for that! If you want to see it in action, you can check it out here (best two open two tabs and chat with yourself):
https://topquiz.at/beta/spectate/0WJqCGDCbPCUNEunLIyyT-9kJ4f3XCc6iMTHtYj._SuTTFcthk

My whole project is going to be open source at some point, so if you see something you like, I'll gladly let you check out my code. (So far I have added reactions and a reply-to feature and changed the emoji-picker positioning).

See you around, cheers, Lukas!

@bLind17
Copy link
Author

bLind17 commented Dec 10, 2020

btw. if you add white-space: pre-wrap; to your text-input you will get rid of the extra line when you delete all the text.

@a-kriya
Copy link
Collaborator

a-kriya commented Dec 10, 2020

@bLind17 Thank you for your response.

If I select some text and then add an emoji, I expect the text to be overwritten, as that is the common behaviour.

Arguably. In the browser, clicking a focusable element (the emoji selector button, in this case) usually clears any selection.

never comprimise on usability to get cleaner code

I shouldn't have really used the word "compromise" in my previous comment as I see this as more of an implementation detail than a usability issue due to the above reason. An example of what complexity can obscure: the event listener added for selectionchange is never removed, so it will leak memory for instances where the widget is used in a single-page app.

By the way, I see the following error when trying to open the chat window at the link you shared and the window does not show up:

image

@bLind17
Copy link
Author

bLind17 commented Dec 11, 2020

Arguably. In the browser, clicking a focusable element (the emoji selector button, in this case) usually clears any selection.

While that may be true, I still feel like a chat window is like a tiny-text-editor and the behaviour makes sense. I checked with the WhatsApp web app and facebook chat, they both do it that way.

I shouldn't have really used the word "compromise" in my previous comment as I see this as more of an implementation detail than a usability issue due to the above reason. An example of what complexity can obscure: the event listener added for selectionchange is never removed, so it will leak memory for instances where the widget is used in a single-page app.

Well of course you are right with that, and I too consider the principle of clean code very imporant. The question is, what's the alternative? Once the focus goes to the emoji picker, the selection is cleared and we lose track of the caret, right?
I'm rather new to web developing, so I lack long term experience and might have missed something, but the selectionchange event was the only means I could find to properly track the selections and therefore the caret.
The blur event would make sense, but I read that the selection will already be cleared once it is fired.

By the way, I see the following error when trying to open the chat window at the link you shared and the window does not show up:

image

Oh, thanks for the info. Works for me on the latest browser versions. I realize that the replaceAll method is only supported rather recently. Could you do me a favour and check your browser version, that would help me a lot, thanks! :)

@a-kriya
Copy link
Collaborator

a-kriya commented Dec 11, 2020

What's the alternative? Once the focus goes to the emoji picker, the selection is cleared and we lose track of the caret, right?

That's true; I concede. The event handler is needed for a proper implementation of this feature.

replaceAll method is only supported rather recently

Starting from Chrome 85, yes. I'm currently on 81, and get left behind as I must manually update Ungoogled Chromium. This one's on me, too.

Thanks for your responses. The additional prop, sendEmojisDirectly, is now the only thing I'm uneasy about. As we know that many popular chat apps behave in this manner, it should be fine to make this the default and, perhaps, the only way. It would only be a breaking change if the handling of message.type === 'emoji' was removed. As that isn't the case, it can be considered an improvement, and released with a minor version bump. I leave the decision to @mattmezza.

I will review the code soon and provide feedback, if any, or approve.

@bLind17
Copy link
Author

bLind17 commented Dec 11, 2020

Let me know what you decide about the default behaviour, I'll gladly remove the sendEmojisDirectly prop and add a line to remove the event handler for the selectionchange event :)

@a-kriya
Copy link
Collaborator

a-kriya commented Dec 12, 2020

Sure. Also just noticed while testing that the undo buffer of the field gets messed up after an emoji is inserted (Cmd/Ctrl + Z). I wonder if that could be addressed if the emoji was inserted using a keyboard event.

@stale
Copy link

stale bot commented Feb 10, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Feb 10, 2021
@stale stale bot closed this Feb 17, 2021
@a-kriya a-kriya reopened this Feb 17, 2021
@stale stale bot removed the stale label Feb 17, 2021
@stale
Copy link

stale bot commented Apr 18, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 18, 2021
@a-kriya a-kriya removed the stale label Apr 19, 2021
@Sitronik
Copy link

Hello @bLind17 I added your functionality to fork for vue 3

@stale stale bot added the stale label Jun 20, 2021
Repository owner deleted a comment from stale bot Jun 21, 2021
@stale stale bot removed the stale label Jun 21, 2021
@stale
Copy link

stale bot commented Aug 20, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Aug 20, 2021
@a-kriya a-kriya added pinned and removed stale labels Aug 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Typing a message without sending, then choosing an emoji will discard the text
3 participants