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 android call next #3088

Closed

Conversation

gracicot
Copy link
Contributor

@gracicot gracicot commented Nov 1, 2019

Is this adding or improving a feature or fixing a bug?

Fixing a bug

What's the new behavior?

When writing such plugin:

function MyPlugin() {
    return {
        onCompositionStart:() => {
            console.log("hello");
        }
    };
}

When starting a composition, "hello" is printed in the console. This works today on desktop.

However, this is not the case for Android. On Android, the Noop plugin was activated, and no hooks would call next.

As stated in noop.js:

IMPORTANT:

This plugin is detached (i.e. there is no way to turn it on in Slate).
You must hard code it into plugins/react/index.

However, it was always activated in dom/index.js:

  const androidPlugins = IS_ANDROID
    ? [AndroidPlugin(options), NoopPlugin(options)]

How does this change work?

I added a switch to turn on or off the noop plugin and added calls to next() in the hooks of the Android plugin

Have you checked that...?

  • 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 prettier.)
  • The relevant examples still work. (Run examples with yarn watch.)

Does this fix any issues or need any specific reviewers?

Fixes: #
Reviewers: @ianstormtaylor

Copy link

@HASSANSTTATI HASSANSTTATI left a comment

Choose a reason for hiding this comment

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

@ianstormtaylor
Copy link
Owner

Hey, there are lots of open Android-related issues that are out of date with the latest 0.50 release because the codebase was completely rearchitected. We'll need to figure out how to have proper support for Android going forward based on beforeinput events somehow. I'm tracking this in #3112, so I'm going to close this in favor of that one.

If it cannot be implemented simply in core with beforeinput we'll likely need to have a separate plugin for android support be created, because it's too much of a departure from all of the other simpler logic in core and very hard for me to test or respond to issues. Using a separate plugin should be possible now as the newest version simplifies a lot of the internals.

Thank you for understanding.

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.

3 participants