Slate: Create process to accept Android PRs

Created on 11 Nov 2019 · 6Comments · Source: ianstormtaylor/slate

Do you want to request a _feature_ or report a _bug_?

Process improvement

What's the current behavior?

We are starting to see PR’s and issues along with a number of people understanding and contributing to the Android plugin that I contributed several months ago. Thank you so much for the people taking the time to understand the code and contribute fixes.

While I’ve provided guidance when asked for and have been answering questions in issues and via Slack, I feel sorry that I haven’t reviewed and accepted any PR’s and at present I'm probably the only one that can accept the PR's.

The purpose of this issue is to figure out a way to start getting these PR's accepted into master.

What's the expected behavior?

The main issue is I have limited time.

The best way to get me to review PRs is to have somebody pre-vet the PRs by doing the following:

Using a virtual machine or an actual mobile phone (preferred) emulate both Android 8 and Android 9. Important to have both.
Execute all the manual composition tests on both machines here https://www.slatejs.org/#/composition/split-join
Ensure that we create a new manual composition test that ensures that the use case that the PR fixes can be confirmed as fixed and also so that future PRs don't accidentally break the fix.

With all these things done then, theoretically, I should be able to run through the tests and accept the PR more quickly.

In the long-term, it would be nice to get those familiar with the Android code to start accepting PRs on their own. Although at that point, I'd probably recommend owning a physical device for Android 8 and Android 9.

Source

thesunny

👍5 🎉1

All 6 comments

I just saw the PR for the next version of Slate https://github.com/ianstormtaylor/slate/pull/3093 and I noticed that at this time, the Android plugin has been removed.

I'm seeking clarification from Ian with respect to Slate and Android support on Slack.

thesunny on 11 Nov 2019

😕3

I strongly agree with this. I opened a lot of pull requests and deploy the changes in our fork, but the changes could benefit a broader amount of users if some of them could be merged.

Also, I think we will need a larger amount of tests. I found a lot of small corner cases that happen with small documents. I'll try to add more examples to test there.

gracicot on 11 Nov 2019

Posting to note that there is now a thread in the contributing channel on Slack where we are discussing how to implement Android support in “next”

thesunny on 11 Nov 2019

👍1

@thesunny annotations do not work on android
@gracicot In ur druid version, annotations do not work either

victorpavlenko on 12 Nov 2019

As ianstormtaylor told me on Slack, the next branch will allow the composition manager to be in a separated repository since there would be no before plugin. This is help shipping updates faster without using our fork.

gracicot on 20 Nov 2019

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 https://github.com/ianstormtaylor/slate/issues/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.