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

Make emoji/text match case insensitive (alternative implementation) #5186

Merged
merged 1 commit into from
Sep 17, 2021
Merged

Make emoji/text match case insensitive (alternative implementation) #5186

merged 1 commit into from
Sep 17, 2021

Conversation

hiqua
Copy link
Contributor

@hiqua hiqua commented Apr 15, 2021 •
edited
Loading

Fixes #5018.

Alternative to #5156.

Contributor checklist:

Description

See issue.

@hiqua hiqua force-pushed the emoji_text_insensitive_alt branch from 8e9d956 to 6fa9fff Compare April 15, 2021 12:54
@hiqua hiqua marked this pull request as draft April 15, 2021 12:54
@hiqua hiqua marked this pull request as ready for review April 15, 2021 12:56
@hiqua hiqua mentioned this pull request Apr 15, 2021
Closed
5 tasks
@hiqua hiqua force-pushed the emoji_text_insensitive_alt branch from 6fa9fff to 312eb15 Compare April 15, 2021 13:24
EvanHahn-Signal
EvanHahn-Signal suggested changes Apr 15, 2021
View reviewed changes
Copy link
Contributor

@EvanHahn-Signal EvanHahn-Signal left a comment

Choose a reason for hiding this comment

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

I prefer this solution. One comment before merging.

Also, I'm not super familiar with this code—how is getBlotTextPartitions used?

ts/quill/util.ts Outdated Show resolved Hide resolved
@hiqua hiqua force-pushed the emoji_text_insensitive_alt branch from 312eb15 to 99af264 Compare April 15, 2021 19:30
@hiqua
Copy link
Contributor Author

hiqua commented Apr 15, 2021

Also, I'm not super familiar with this code—how is getBlotTextPartitions used?

Given that I have only a superficial understanding of quill and these blot objects, I don't think I'll be able to give you more information than what you can get by reading the two calls it has in the code (in ts/quill/util.ts and ts/quill/emoji/completion.tsx). But it just splits text around an index. Or are there more specific unknowns you want to address before considering merging this?

@hiqua
Copy link
Contributor Author

hiqua commented May 13, 2021 •
edited
Loading

@EvanHahn-Signal what would be the way forward with this PR?

@EvanHahn-Signal
Copy link
Contributor

Sorry. We've been really busy and haven't had time to take a look at this. It's still on our list.

No further action needed from you, unless you want to rebase your work off of the latest development if you haven't already.

@hiqua hiqua force-pushed the emoji_text_insensitive_alt branch from 99af264 to 12b2d9c Compare May 14, 2021 07:57
@hiqua
Copy link
Contributor Author

hiqua commented May 14, 2021

Ok great! I've rebased it.

@hiqua hiqua mentioned this pull request Jul 2, 2021
Open
@hiqua
Copy link
Contributor Author

hiqua commented Aug 2, 2021

@EvanHahn-Signal could you guys have a look sometime soon?

@EvanHahn-Signal
Copy link
Contributor

Apologies. I have no updates here, but I'll discuss with the team.

@EvanHahn-Signal EvanHahn-Signal changed the base branch from development to merge-5186 September 17, 2021 16:19
EvanHahn-Signal
EvanHahn-Signal approved these changes Sep 17, 2021
View reviewed changes
Copy link
Contributor

@EvanHahn-Signal EvanHahn-Signal left a comment

Choose a reason for hiding this comment

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

Sorry this took so long to review. Merging now! This should go into the next release.

@EvanHahn-Signal EvanHahn-Signal merged commit 1aab415 into signalapp:merge-5186 Sep 17, 2021
@EvanHahn-Signal EvanHahn-Signal mentioned this pull request Sep 17, 2021
Closed
1 task
@hiqua
Copy link
Contributor Author

hiqua commented Sep 19, 2021

Awesome, thanks!

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

@EvanHahn-Signal EvanHahn-Signal EvanHahn-Signal approved these changes

Assignees
No one assigned
Labels
PR: Needs Review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot select an emoji when using a capital letter
2 participants
@hiqua @EvanHahn-Signal