Generated by 🚫 dangerJS

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

You can test the changes on this Pull Request by downloading the APK here.

Testing build 78558 and it's looking great! I understand this is a draft but I wanted to do some exploratory testing to make sure I have WPiOS aligned with this.

There are some differences between the platforms in how @-mentions was implemented that carry over now into x-posts. I'll list the ones I've noticed here since now would be a good time to unify this:

• There's a "No matching crosspost." message shown when what the user types doesn't match any results from the xpost suggestion list (iOS doesn't show anything at the moment) — the same applies to @-mentions where a "No matching users." message is shown on WPAndroid but not on WPiOS
• If I then go ahead and hit the keyboard Done button while the "No matching users." message is shown, I see a "@xyz is not a valid user" toast message (same applies to xposts where I see a "+xyz is not a valid crosspost")

I think the above should be added to WPiOS, for both @-mentions and xposts, so I created wordpress-mobile/WordPress-iOS#15148.

A couple of things I noticed some of which you might be already aware of:

• On sites which don't have the O2 plugin (e.g. a brand new WordPress.com simple site) the "Loading..." message is shown for a minute or so before disappearing (I'm reasonably sure the server responds quickly in this scenario with an error message)
• The editor freezes or crashes (without crashing the app itself) about 1 out of 3 times when I'm on a site that doesn't support xposts, but I type + and then backspace to delete the + character while the "Loading..." message is shown
  View screen recording

  

👋 @guarani ! Thanks so much for doing such thorough testing! 🙇 I've made some code changes and would appreciate if you would give the latest build another run through to see if there are any remaining issues.

The editor freezes or crashes (without crashing the app itself) about 1 out of 3 times when I'm on a site that doesn't support xposts, but I type + and then backspace to delete the + character while the "Loading..." message is shown

I don't think you'll see this anymore, but if you do, let's see if we can pull the stacktrace for the crash.

Tested build 80467 and it's looking good!

I don't think you'll see this anymore, but if you do, let's see if we can pull the stacktrace for the crash.

I haven't seen this on the new build, so it looks to be resolved.

Common use cases are working great:

• Cross posts worked between P2 sites
• A new site added to my account became available in the xpost suggestion UI
• A site deleted from my account was no longer in the xpost suggestion UI
• I could add xposts to a post while offline using cached data

I've noticed a couple of differences between the app an mobile and the web (although I'm quite sure they're not unique to the Android implementation, it's likely the same on the iOS side):

• I have two sites that can xpost to each other using the app, but on WP.com no xpost options appear. Both are P2 sites, however one was created using an A8c account while the other was created using a regular WP.com site. I use the same user, added as an Admin on both sites, to test.
• The web appears to allow Super Admin users to xposts between sites within the same WordPress Multisite network while the mobile implementation does not.

Mobile-specific:

• xposts only work on the first line of pre-formatted blocks (e.g. Verse, Preformatted) — the same bug applies to @-mentions
  See screen recording:

  

Android-specific:

• In landscape orientation, typing + appears to show the suggestions UI off-screen. I don't think this is a blocker though, because landscape mode already feels hard to use because of the white text area shown instead of the regular editor UI:
  See screen recording:

  

There's a lot here that I know applies to iOS as well (and some points I haven't tested on iOS yet), so I'll be looking into some of these as part of the iOS implementation as well.

Thanks so much for the thorough review @guarani ! 🙇

I have two sites that can xpost to each other using the app, but on WP.com no xpost options appear. Both are P2 sites, however one was created using an A8c account while the other was created using a regular WP.com site. I use the same user, added as an Admin on both sites, to test.

I'm having some trouble recreating this issue. I have two P2 sites, one was created with my A8c account and the other was created with a random non-A8c account. I added both accounts to both P2s, and both accounts can cross-post from either P2 to the other for me. Let me know if you see where I got my setup wrong.

Even if this is an issue, it sounds like the app's behavior would be the correct behavior. Do you agree?

The web appears to allow Super Admin users to xposts between sites within the same WordPress Multisite network while the mobile implementation does not.

I'm not sure exactly what you mean here. Would you mind explaining this a bit more for me?

xposts only work on the first line of pre-formatted blocks (e.g. Verse, Preformatted) — the same bug applies to @-mentions

Good catch. It looks to me like this is just an issue at the beginning of a line that is not the first line in the block. Basically, we were only checking if the trigger key was at the beginning of the text or following a space. I updated the gutenberg code to also check if the previous character was a \n, which I believe addresses this (we should test this on iOS as well to confirm it works there too).

In landscape orientation, typing + appears to show the suggestions UI off-screen. I don't think this is a blocker though, because landscape mode already feels hard to use because of the white text area shown instead of the regular editor UI

Another good catch 🙂 . I've pushed an update (a4d5b4a) that is certainly not a perfect fix, but it does make the UI usable in landscape mode. I think this should be good enough for our initial release, but let me know what you think (also, pinging @iamthomasbishop since you may have thoughts about this).

I'm having some trouble recreating this issue. I have two P2 sites, one was created with my A8c account and the other was created with a random non-A8c account. I added both accounts to both P2s, and both accounts can cross-post from either P2 to the other for me. Let me know if you see where I got my setup wrong.

I factored out the A8c-aspect of this particular issue by reproducing using non-a8c accounts. As you suggested via DM, I'd say this is a bug on the web side (the mobile behavior looks correct). Here's what I did:

1. I created two P2 Blog sites: Site A using WP.com User A and Site B using WP.com User B — both are private.
2. User A was then added to Site B and User B to Site A.

I saw the following behavior:

• I could not xpost between the sites using Gutenberg in Calypso (e.g. logged into Site A using User A, typing + in the editor did not show a result for Site B)
• I could xpost between the sites using the site's frontend editors (that's to say, by going to the site's address instead of https://wordpress.com/post/<site address>)
• I could also xpost between the sites using WPAndroid.

I think the next step here would be to report this as a bug in Gutenberg Calypso, do you agree?

The web appears to allow Super Admin users to xpost between sites within the same WordPress Multisite network while the mobile implementation does not.

I'm not sure exactly what you mean here. Would you mind explaining this a bit more for me?

Sorry for the confusion. From what I can see, if I am a Super Admin on a Multisite network, on the web (both in Calypso Gutenberg and P2 frontends), I can xpost to any of the sites on the network, even if I'm not a member (admin/editor/subscriber/etc) of that site. On mobile however, I can only xpost to sites where I'm a member of the site.

I updated the gutenberg code to also check if the previous character was a \n, which I believe addresses this (we should test this on iOS as well to confirm it works there too).

Thanks a lot for making a fix!

I've pushed an update (a4d5b4a) that is certainly not a perfect fix, but it does make the UI usable in landscape mode.

Awesome! From the gif it looks like a great improvement.

certainly not a perfect fix, but it does make the UI usable in landscape mode

@mchowning Those were my exact my thoughts when seeing your example gif. Does this also apply to the at-mentions UI? I'm assuming yes but wanted to make sure 😄

Does this also apply to the at-mentions UI? I'm assuming yes but wanted to make sure 😄

That's right @iamthomasbishop . Both the problem and the better-but-certainly-not-ideal-fix apply to both the at-mentions and xpost UIs. Also, fwiw, the landscape UI works fine on a tablet, so this was only an issue on phone screens (where I assume using landscape mode would be much less common).

Temporarily updating this pr to target the gutenberg/after_1.41.0 branch so it doesn't show a bunch of extra commits. Will make sure this is retargeted to develop once that branch is merged.

UPDATE: now switched back to develop

Tests cases are all looking great for me, including landscape mode, and cross posts/mentions after new lines in multiline blocks. I'll take another day or two to give the code updates in this PR and related PRs a good review. From what I've looked at so far, code looks great, awesome job on this feature.

@mchowning I had a good look at the code across all three repos and do not have any remaining concerns. (One question on FluxC PR, but it's minor and if anything just potential for reducing minor code duplication, if that)

I did extra testing around edge cases coming on and offline, and different cases with xposts and mentions where having one matching result or none and the feature produced the expected outcomes each time. And I tested with one site that had a suggestion list that didnt block the whole screen, and that was dismissable as expected, so everything is looking good. Switching between landscape and portrait in different states with the suggestions activity open looked good as well.

Before I approve, can you confirm if there is another change coming related to tracking user behavior? If not, I'll approve. Again, super well executed, was a pleasure to review. 💯 ⭐

Thanks for doing such a thorough job testing this PR @cameronvoell !

can you confirm if there is another change coming related to tracking user behavior?

There are additional changes for tracking, but I'm keeping them in a separate PR because I didn't want to complicate this already-too-big PR.

Thanks for the review Paul!

Looks like the only pending item here is to either add an item to the release notes to enable this only for dev builds.

I don't think there's any need to limit this to dev builds, so I updated the release notes. Let me know if you have any concerns about that.

There are additional changes for tracking, but I'm keeping them in a separate PR because I didn't want to complicate this already-too-big PR.

Since both PRs are now approved, I went ahead an merged the analytics PR into this branch (91cb68b) so we will just have a single merge to develop.

Gave this a quick test with the latest apk and all looks good! Merging now.