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

fix(NcRichContenteditable): Fix tribute emoji complete interfering unexpectedly aka. 🇨🇨 #3924

Merged
merged 2 commits into from Mar 24, 2023
Merged

fix(NcRichContenteditable): Fix tribute emoji complete interfering unexpectedly aka. 🇨🇨 #3924

merged 2 commits into from Mar 24, 2023

Conversation

nickvergessen
Copy link
Contributor

@nickvergessen nickvergessen commented Mar 24, 2023 •
edited

🪦 Cocos-Island Meme

Bildschirmaufzeichnung vom 2023-03-24, 07-16-01

Opposed to #3923 I fixed it in NcRichContenteditable so that other components like the emoji picker itself are not interfered and still operate normally.

@nickvergessen nickvergessen added bug Something isn't working 3. to review Waiting for reviews papercut Annoying recurring issue with possibly simple fix. feature: rich-contenteditable Related to the rich-contenteditable components labels Mar 24, 2023
@nickvergessen nickvergessen added this to the 7.8.4 milestone Mar 24, 2023
@nickvergessen nickvergessen self-assigned this Mar 24, 2023
skjnldsv
skjnldsv reviewed Mar 24, 2023
View reviewed changes
Copy link
Contributor

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments

src/components/NcRichContenteditable/NcRichContenteditable.vue Outdated Show resolved Hide resolved
src/components/NcRichContenteditable/NcRichContenteditable.vue Outdated Show resolved Hide resolved
src/components/NcRichContenteditable/NcRichContenteditable.vue Outdated Show resolved Hide resolved
@nickvergessen
fix(NcRichContenteditable): Fix tribute emoji complete interfering un…
2b371da 
…expectedly aka. Cocos Island Meme

Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen force-pushed the bugfix/3825/fix-emoji-autocomplete branch from 28f4500 to 2b371da Compare March 24, 2023 06:49
@nickvergessen
fix(NcRichContenteditable): Also fix lowercase character text smiles
c1b9a21 
Signed-off-by: Joas Schilling <coding@schilljs.com>
skjnldsv
skjnldsv approved these changes Mar 24, 2023
View reviewed changes
Copy link
Contributor

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

ChristophWurst
ChristophWurst approved these changes Mar 24, 2023
View reviewed changes
Copy link
Contributor

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

mejo-
mejo- requested changes Mar 24, 2023
View reviewed changes
Copy link
Contributor

@mejo- mejo- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, I think that #3923 is the cleaner approach. It doesn't affect the EmojiPicker component either, the search function is only used by emoji autocompletion implementions in NcRichContenteditable and Text/Collectives. That's also why I think that it's the proper place to fix this.

mejo-
mejo- approved these changes Mar 24, 2023
View reviewed changes
Copy link
Contributor

@mejo- mejo- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get this in nevertheless.

@nickvergessen nickvergessen merged commit 43641e0 into master Mar 24, 2023
15 checks passed
@nickvergessen nickvergessen deleted the bugfix/3825/fix-emoji-autocomplete branch March 24, 2023 09:00
@nickvergessen
Copy link
Contributor Author

After some discussion in the chat we agreed to go with both approaches in parallel:

  1. This PR here is a UX fix to help users in cases it was not in the intention of the user to autocomplete anything at all but the Enter was planned to submit the input
  2. The PR feat(Emoji): Suggest emojis based on text smiles #3923 is a nice UX improvement to suggest emojis for text smiles so that users can autocomplete them if they want to, but the default behaviour is to keep the impact on users that do NOT wish to do so as low as possible

This was referenced Mar 24, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@mejo- mejo- mejo- approved these changes

@skjnldsv skjnldsv skjnldsv approved these changes

@ShGKme ShGKme Awaiting requested review from ShGKme

@marcoambrosini marcoambrosini Awaiting requested review from marcoambrosini

@Antreesy Antreesy Awaiting requested review from Antreesy

Assignees

@nickvergessen nickvergessen

Labels
3. to review Waiting for reviews bug Something isn't working feature: rich-contenteditable Related to the rich-contenteditable components papercut Annoying recurring issue with possibly simple fix.
Projects
None yet
Milestone
7.8.4
Development

Successfully merging this pull request may close these issues.

Emoji picker suggestions are unexpected when using ":)" and ":D"
4 participants
@nickvergessen @ChristophWurst @mejo- @skjnldsv