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

@-mention Activity for Gutenberg #11693

Merged
merged 23 commits into from
May 20, 2020

Conversation

mchowning
Copy link
Contributor

@mchowning mchowning commented Apr 17, 2020

Related gutenberg-mobile PR: wordpress-mobile/gutenberg-mobile#2163

Adds @-mention support in the Gutenberg editor. Also updates the @-mention support throughout WPAndroid to show the popup immediately after the @-symbol is shown instead of waiting until an "@" plus a single letter is shown. This change brings Android in line with the behavior on iOS.

Slow Initial Loading of Suggestions

The first time the suggestions are loaded for a site (which is done the first time a SuggestionAutocompleteTextView is displayed, the suggestions are very slow to load. In my testing, it would take 20-30 seconds before the suggestions would be available to the user.

One cause of this is the slowness of the API call for sites with many users. In my tests the api call for a site with 1500+ users took 3 to 6 seconds. A site with only a single user returned in well under a second.

The larger problem, however, is that inserting those suggestions into the database is taking >15 seconds. I have a separate PR to address that delay, but that fix is not included in this PR, so expect a significant delay the first time the suggestions are loaded for a particular site.

Testing

Gutenberg

Inserting mentions:

  1. Edit a post using Gutenberg
  2. Open any RichText-based block.
  3. Tap the @ button on the far right of the format toolbar
  4. Either
  5. Tap on one of the mentions suggested by the UI or
  6. Enter enough text to filter down to a single suggestion and then press the done button on the keyboard
  7. Observe the appropriate mention is inserted into the block

Does not insert with keyboard done button if there is anything but a single suggestion

  1. Edit a post using Gutenberg
  2. Open any RichText-based block.
  3. Tap the @ button on the far right of the format toolbar
  4. Enter text to filter to either 0 or >2 mentions (so not 1)
  5. Tap the keyboard's done button
  6. Observe a toast message advising that the user has not entered a valid mention.

WPAndroid

  1. Add or respond to a comment using the app
  2. Observe that the @-mention popup occurs after typing the @ symbol instead of needing @ + a letter to reveal the suggestions.

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@mchowning mchowning self-assigned this Apr 17, 2020
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Apr 17, 2020

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

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Apr 17, 2020

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

@mchowning mchowning added this to the 14.7 milestone Apr 20, 2020
@mchowning mchowning force-pushed the gutenberg/issue_331/mention_support branch from ecef710 to dc61f32 Compare April 20, 2020 16:11
@mchowning mchowning marked this pull request as ready for review April 20, 2020 17:19
@mchowning mchowning force-pushed the gutenberg/issue_331/mention_support branch from dc61f32 to 85f416b Compare April 20, 2020 18:35
@mchowning mchowning changed the base branch from gutenberg/after_1.26.0 to develop April 20, 2020 19:58
@oguzkocer
Copy link
Contributor

Hi there! 14.7 is being frozen, so this PR is being bumped to 14.8. If these changes have to be shipped with 14.7, please target release/14.7 and cc me in the PR so I can cut a new beta release. Also if you can keep the release dates in mind and bump the PRs & issues you are working on before the code freeze date, we'd really appreciate it.

P.S: I am still working on the messaging for these bumps, so please let me know if you have any questions/concerns/feedback.

@oguzkocer oguzkocer modified the milestones: 14.7, 14.8 Apr 20, 2020
@mchowning mchowning marked this pull request as draft April 24, 2020 20:21
@mchowning mchowning force-pushed the gutenberg/issue_331/mention_support branch from 3baf0d4 to cb035c5 Compare April 28, 2020 16:55
@mchowning mchowning force-pushed the gutenberg/issue_331/mention_support branch from fcf39fc to 077881e Compare April 29, 2020 13:11
@mchowning mchowning marked this pull request as ready for review April 29, 2020 13:12
Avoids an issue where the empty view appears when it shouldn't at times.
For example if you press the background when there is a single
suggestion and don't lift your finger, the suggestion disappears, but
the activity doesn't close until you left your finger, so the empty view
becomes visible as long as you hold your finger down.
@mchowning mchowning force-pushed the gutenberg/issue_331/mention_support branch from 077881e to 4abaf71 Compare April 29, 2020 13:22
@hypest hypest removed their request for review April 30, 2020 08:22
@oguzkocer
Copy link
Contributor

Hi there! 14.8 is being frozen, so this PR is being bumped to 14.9. If these changes have to be shipped with 14.8, please target release/14.7 and cc me in the PR so I can cut a new beta release. Also if you can keep the release dates in mind and bump the PRs & issues you are working on before the code freeze date, we'd really appreciate it.

@oguzkocer oguzkocer modified the milestones: 14.8, 14.9 May 4, 2020
Copy link
Contributor

@cameronvoell cameronvoell left a comment

Choose a reason for hiding this comment

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

I tested the main cases from the description and they look good.

A super minor issue is that if you rotate while the @ menu is open, and then select an option, I do not see it get added. If you then try a second time it works. Since it doesn't cause any bad state, I don't see it as a blocker, maybe something to look at in future improvements.

I saw you have a PR up for enabling a space after the @ mention. I'll take a look at that next. Nice job on this, especially the nice Java 8 method reference refactor 🎉 .

(edit - minor issue that I think is not a blocker, video on gutenberg PR WordPress/gutenberg#21651 (review))

mEditorFragmentListener::onAuthHeaderRequested,
mEditorFragmentListener::onPerformFetch,
mEditorImagePreviewListener::onImagePreviewRequested,
mEditorEditMediaListener::onMediaEditorRequested,
Copy link
Contributor

Choose a reason for hiding this comment

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

Love how concise this is with Java 8 double colon method reference 👍

@@ -16,6 +16,8 @@
* Added user Gravatar to the Me menu in My Site.
* Updated site details screen title to "My site", to avoid duplicating the title of the current site which is displayed in the screen's header area.

* @-mention suggestions are now presented immediately after typing "@"
Copy link
Contributor

@cameronvoell cameronvoell May 7, 2020

Choose a reason for hiding this comment

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

Maybe change from "typing" to "entering" or "pressing" "@" since the ability to type "@" on the keyboard is a design request that is not yet implemented. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I think you're right! I actually need to remove this entirely for now though because we're going to keep this under a dev flag for the time being.

.startsWith(lowerCaseConstraint)
|| suggestion.getDisplayName().toLowerCase(Locale.getDefault())
.contains(" " + lowerCaseConstraint);
if (suggestionMatchesConstraint) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Almost suggested a unit test here, but then saw it was really just three intuitive cases, and I tested to make sure they all work fine. Looks good

@mchowning mchowning modified the milestones: 14.9, 15.0 May 11, 2020
@SergioEstevao SergioEstevao merged commit 5ed5d7f into develop May 20, 2020
@SergioEstevao SergioEstevao deleted the gutenberg/issue_331/mention_support branch May 20, 2020 10:43
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.

4 participants