-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Add unicode sequences support #4326
Conversation
🦋 Changeset detectedLatest commit: f584aeb The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I think this is really amazing. I gave a few specific questions and feedback.
One note, parts of this change are very specific to knowledge about unicode, so more comments and references would be a good idea in my opinion.
I'm happy to review again after you've had time to address my suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @oliger, this looks great to me. I'm going to leave it open for a day or two in case anyone else has suggestions before we land it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oliger please review the failing test, thanks!
Hello @dylans! Thanks for the review. A made some requested changes and left a comment. Concerning the comments I do agree that a bit more would be better. I am also sure that the code could be edited to be more readable... But unfortunately, I do not have the time to focus on this at the moment. |
Looking at why the tests are failing... This is curious 🤔 |
c1ba914
to
651b9fc
Compare
@dylans tests are back to green. I also fixed a mistake I made while resolving conflict. |
* Add failing test * Handle sequences * Uncomment test cases * Handle RTL unicode sequences * Remove esrever * Add tests * Use iterator instead of Array.from * Add changeset * Rename split to splitByCharacterDistance * Make reverse optional * Fix casing * Fix yarn.lock * Fix tests * Remove fast-deep-equal after bad merge
@oliger How well do you remember all the unicode logic here? I think you broke one case: variation selector, because I'm pretty sure that my initial unicode logic supported it. Try to paste them here: https://www.slatejs.org/examples/richtext Both emojis end with the following variation selector: https://emojipedia.org/emoji/%EF%B8%8F/ |
@gitcatrat Hello, it seems that the issue is related some BMP emojis not being white listed in I have no idea if it was working before and I cannot test as I have limited access to a computer at the moment. I am not sure if I'll have the time to focus on this anytime soon. |
* Add failing test * Handle sequences * Uncomment test cases * Handle RTL unicode sequences * Remove esrever * Add tests * Use iterator instead of Array.from * Add changeset * Rename split to splitByCharacterDistance * Make reverse optional * Fix casing * Fix yarn.lock * Fix tests * Remove fast-deep-equal after bad merge
Description
This PR follows up on #4305. It adds support for flag, keycap and tag unicode sequences.
Example
Without this PR:
CleanShot.2021-06-08.at.17.37.13.mp4
With this PR:
CleanShot.2021-06-08.at.17.39.47.mp4
Context
Improves
getCharacterDistance
so that it handles all Unicode sequences. Stop reversing the string when moving from right to left. Split it by codepoint and iterate from first to last when moving from left to right and iterate from last to first when moving from right to left.Checks
yarn test
.yarn lint
. (Fix errors withyarn fix
.)yarn start
.)yarn changeset add
.)