vue-beautiful-chat icon indicating copy to clipboard operation
vue-beautiful-chat copied to clipboard

Fixes #209: Allow inserting emojis alongside text

Open bLind17 opened this issue 4 years ago • 12 comments

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.

bLind17 avatar Nov 11 '20 18:11 bLind17

@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 avatar Dec 09 '20 20:12 a-kriya

* 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 avatar Dec 10 '20 18:12 bLind17

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.

bLind17 avatar Dec 10 '20 18:12 bLind17

@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

a-kriya avatar Dec 10 '20 19:12 a-kriya

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! :)

bLind17 avatar Dec 11 '20 08:12 bLind17

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.

a-kriya avatar Dec 11 '20 21:12 a-kriya

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 :)

bLind17 avatar Dec 11 '20 22:12 bLind17

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.

a-kriya avatar Dec 12 '20 07:12 a-kriya

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[bot] avatar Feb 10 '21 08:02 stale[bot]

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[bot] avatar Apr 18 '21 13:04 stale[bot]

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

Sitronik avatar Apr 21 '21 14:04 Sitronik

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[bot] avatar Aug 20 '21 11:08 stale[bot]