Add keyboard shortcut for emoji reactions #5425
Conversation
Signed-off-by: macekj <macekj@umich.edu>
|
Very interesting! |
|
featureswise/designwise this is great - lgtm :) |
| @@ -47,7 +47,7 @@ import AutocompleteWrapperModel from "../../../editor/autocomplete"; | |||
| import DocumentPosition from "../../../editor/position"; | |||
| import {ICompletion} from "../../../autocomplete/Autocompleter"; | |||
|
|
|||
| const REGEX_EMOTICON_WHITESPACE = new RegExp('(?:^|\\s)(' + EMOTICON_REGEX.source + ')\\s$'); | |||
| const REGEX_EMOTICON_WHITESPACE = new RegExp('(?:^|\\s|(?<=^\\+))(' + EMOTICON_REGEX.source + ')\\s$'); | |||
There was a problem hiding this comment.
this regexp is getting quite impenetrable now :) can we comment it?
// match emoticons which follow the start of a line, whitespace, a plus at the start of a line.
I'm a bit unclear on why we need the nested positive look-behind assertion here - plus it looks like the (?<=) will act as a capturing group, which might have undesirable side-effects given the wrapping group is a non-capturing (?:). Does (?:^|\\s|^\\+) work?
There was a problem hiding this comment.
Good point! I've gone ahead and commented it as well as removed the lookbehind and it is still working as intended.
I think the negative lookbehind is still necessary for the EmojiProvider regex. Otherwise, the entire match gets replaced when autocompleting an emoji suggestion ("+:gleeful" gets sent to "😃" instead of "+😃"). Unless there is a different way to do it.
There was a problem hiding this comment.
thanks for this (and thanks for the PR!). I guess a similar clarification is needed for EMOJI_REGEX then to comment why the lookbehind is needed there but not here?
otherwise this lgtm, but i'll defer to @jryans & co for formal review from the eleweb team.
Signed-off-by: macekj <macekj@umich.edu>
jryans
requested review from
jryans and
bwindels
and removed request for
a team
Signed-off-by: macekj <macekj@umich.edu>
|
I did some more digging and testing with the regexes and it turns out that neither of them actually play into the autocompletion, and the feature works as intended without having modified either regex! I must have changed them early on thinking it would be necessary, but it looks like the only necessary change is looking for a "+" in tabCompleteName. Hopefully this is more clear. Thank you for the feedback so far! |
|
@macekj I think the only step remaining is to get tests to pass. I think the error is unrelated to your changes, and will be fixed if you merge in the latest changes from the develop branch. |
|
Doing a gentle bump -- I merged from develop last week, if there's anything else I need to do just let me know! 😄 |
|
Thanks for the comment! (Do feel free to message on a PR like you did and / or in #element-dev wherever it feels like things have been stuck for several days, as it may have just gotten stuck in the tubes full of GH notifications.) 😄 Looks like we're all ready to go here, so let's merge! Thanks again for working on this! 😄 |
|
A follow up issue to document this awesome shortcut: |
Fixes element-hq/element-web#10343
Allows for users to quickly react to the most recently sent message in the current room by typing + followed by an emoji into the message composer. This is a feature supported by both Discord and Slack.
This is my first contribution to this project, so please let me know if there is anything I need to do 😄
Signed-off-by: Joe Macek macekj@umich.edu