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.

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

Fix toolbar and mentions examples on android #5071

Conversation

BitPhinix
Copy link
Contributor

Description
Updates the mention and hovering toolbar example so that they properly work on android.

This PR also contains a potentially breaking change: It memos the Editable component to make it easier to work with on android since renders cause a flush which will mess with the IME input.

I'm aware that there is still an issue when using swiftkey in the mention example. Swiftkey will duplicate the text inside the mention when pressing space directly after a mention component. This is (one of many) bugs with swiftkey itself and I haven't yet found a way to prevent it (also happens in a non-slate contenteditable, and every other editor out there that is based upon it)

Checks

  • The new code matches the existing patterns and styles.
  • The tests pass with yarn test.
  • The linter passes with yarn lint. (Fix errors with yarn fix.)
  • The relevant examples still work. (Run examples with yarn start.)
  • You've added a changeset if changing functionality. (Add one with yarn changeset add.)

@changeset-bot
Copy link

changeset-bot bot commented Aug 3, 2022

🦋 Changeset detected

Latest commit: 1a94e41

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
slate-react Patch

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

@BitPhinix
Copy link
Contributor Author

@dylans, I've "refactored" the pending diff handling a bit; it should now be nicer to work with. Not a perfect implementation currently, but I'll clean it up if you like the approach here. We could use the same API for desktop composition, although another rabbit hole I haven't looked into yet.

@bryanph
Copy link
Contributor

bryanph commented Aug 4, 2022

@BitPhinix could you explain a little how the shouldFlushPendingChanges method works, when it should be implemented, when it should not be implemented and how it should be modified to make android work in different use cases? 🙏

Also do you use a particular emulator to test with android? Or do you use a physical device.

@BitPhinix
Copy link
Contributor Author

BitPhinix commented Aug 4, 2022

Sure, there are mainly two use-cases for it:

  1. Cancel composition to perform some action on the pending editor state while the user is composing (mainly relevant for auto-replace shortcuts like the markdown shortcut example)
  2. Update some component/state to reflect the pending state while the user is composing (like in the mention example)

It's invoked with a "safe" timing after each composition change, so you could modify the editor state inside it without interfering too much with the IME (I say too much because it still will if the render takes long/you type very fast). Forcing a flush manually by returning true will have these issues as well, so you should use it very sparingly and only where you have no other choice (like with markdown shortcuts).

We never had an API to deal with/update the editor state while the user is composing since the use-case is rare on desktop. We currently just insert the text on composition end. It's needed on android because composition is very long-lived (you can write an entire paragraph on android inside a single composition session).

I'm not super set on the API; there might be a more elegant way to do this. This is the most elegant solution I've come up with for now.

@BitPhinix
Copy link
Contributor Author

Any objections/comments on this, or should I create a polished version of this PR?

@dylans
Copy link
Collaborator

dylans commented Aug 9, 2022

Any objections/comments on this, or should I create a polished version of this PR?

My instinct is that I wish this didn't feel so complex, but I don't have a better suggestion. Let's go for the polished PR. Thanks!

@p1scess
Copy link

p1scess commented Nov 30, 2022

I had the same problem.When will the content of this branch be officially released?Thanks.

@Sunshine168
Copy link

Any update?

@dylans
Copy link
Collaborator

dylans commented Dec 16, 2023

We've had a few smaller PRs to fix parts of this. Closing in favor of a hopefully smaller PR later to address any remaining issues.

@dylans dylans closed this Dec 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants