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

Issue 331/mention support on @ keypress #2218

Closed

Conversation

SergioEstevao
Copy link
Contributor

@SergioEstevao SergioEstevao commented May 5, 2020

Fixes #331

Related PRs:

This PR changes the AztecView component to be able to intercept any keycode that is defined. It then configures uses this functionality to intercept the @ keypress and start the mention UI.

To test:

  • Start the demo app
  • Add a Paragraph block
  • Tap the @ on the keyboard
  • Check if a mention to matt is added
  • Check if the mention is only inserted if the @ is inserted with a space before or at the start of the paragraph.

PR submission checklist:

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

@SergioEstevao SergioEstevao added the [Type] Enhancement Improves a current area of the editor label May 5, 2020
Base automatically changed from issue_331/mention_support to develop May 20, 2020 10:40
…ress

# Conflicts:
#	bundle/android/App.js
#	bundle/android/App.js.map
#	bundle/ios/App.js
#	bundle/ios/App.js.map
#	gutenberg
@@ -165,14 +181,15 @@ class AztecView extends React.Component {
onContentSizeChange = { this._onContentSizeChange }
onHTMLContentWithCursor = { this._onHTMLContentWithCursor }
onSelectionChange = { this._onSelectionChange }
onEnter = { this.props.onEnter && this._onEnter }
onEnter = { this.props.onKeyDown && this._onEnter }
onBackspace = { this.props.onKeyDown && this._onBackspace }
Copy link
Contributor Author

@SergioEstevao SergioEstevao May 21, 2020

Choose a reason for hiding this comment

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

After the Android Aztec lib code is updated the onEnter and onBackspace props should be removed, and all "keypress" events should go trough onKeyDown

@mchowning mchowning force-pushed the issue_331/mention_support_trigger_on_keypress branch from 730566e to 91c1a93 Compare June 4, 2020 15:00
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Jun 4, 2020

Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job!

@SergioEstevao
Copy link
Contributor Author

Tested this on Android and is working great!

@iamthomasbishop do you mind review the Android version to see if the design of the mentions view UI/UX is good?

@iamthomasbishop
Copy link
Contributor

@SergioEstevao Just tested a build on Android and the key press support is working as expected — nice work y’all! 👏

I noted one concern regarding the animation/transition as the height quickly adjusts, causing a gap between the text input row and the suggestions UI, but @mchowning is looking into it and it’s not necessarily a huge blocker.

@iamthomasbishop
Copy link
Contributor

@mchowning @SergioEstevao I had another chance to test on Android in more detail. Here are some notes:

  • I'm not seeing the @ icon-button in the block toolbar — not sure if this was intentionally removed, but I would like to keep it
  • There is currently a short fadeOut animation on the text input row when tapping on a result from the suggestions list. Can we hide the text input row immediately?
  • There's a subtle "flash" on the keyboard when you tap a suggestion (video)
  • Can we add a 1px border-top to the text input row, using the same border color as the block toolbar? Think we had that at some point but it must’ve been removed while we iterated style.

I’d also I’ll Ike to cross-check on iOS to see if any of these issues exist on iOS (particularly the first one). @SergioEstevao are you able to make a new test build?

@SergioEstevao
Copy link
Contributor Author

SergioEstevao commented Jun 12, 2020 via email

…ress

# Conflicts:
#	bundle/android/App.js
#	bundle/android/App.js.map
#	bundle/ios/App.js
#	bundle/ios/App.js.map
#	gutenberg
@iamthomasbishop
Copy link
Contributor

@SergioEstevao Tested iOS and it is looking solid — not seeing the aforementioned issues there. 👍

@mchowning
Copy link
Contributor

👋 @iamthomasbishop . Thanks for taking a look. 🙇 I've addressed your comments and there's a new build you can test. Care to take another look and let me know how things are looking now?

I'm not seeing the @ icon-button in the block toolbar — not sure if this was intentionally removed, but I would like to keep it

As Sergio noted, that was because we had kept the button behind a dev flag until now. That has been removed in the latest code though.

There is currently a short fadeOut animation on the text input row when tapping on a result from the suggestions list. Can we hide the text input row immediately?
...
Can we add a 1px border-top to the text input row, using the same border color as the block toolbar? Think we had that at some point but it must’ve been removed while we iterated style.

Removed the animation and added the top border back.

There's a subtle "flash" on the keyboard when you tap a suggestion (video)

Also took care of this. There is still a small keyboard flash when opening the mentions UI, but it's quite small (at least in my testing) and I think resolving that flash will probably be difficult. If this is something we have to fix before we can release this though, just let me know.

@iamthomasbishop
Copy link
Contributor

@mchowning Most everything is looking solid, just a couple of things.

  • The border on the input row doesn't match the block toolbar in light mode (example of the color it should be). There shouldn't be a border in dark mode (or it can be transparent)
  • In dark mode, the background color should match the block toolbar background (example)
  • I noticed that when you start typing and the suggestions list narrows down from many to 1, the "no matching users available" label is still visible behind the UI. Try typing "matt", waiting then typing "c" — when the results narrow to 1, you'll see the label still visible behind it

I'm not seeing that "selected" flash anymore, nice fix! I'm also not seeing any strong flashing while opening the mentions UI, at least during this quick test.

@mchowning
Copy link
Contributor

@mchowning Most everything is looking solid, just a couple of things...

Thanks @iamthomasbishop. I believe I've addressed those issues, and there's a new build available to test. Let me know what you think!

@mchowning mchowning modified the milestones: 1.31, 1.32 Jun 18, 2020
@iamthomasbishop
Copy link
Contributor

@mchowning had a chance to take the latest build for a spin and it's looking solid. I think we are in good shape to ship it! 😁

…ress

# Conflicts:
#	bundle/android/App.js
#	bundle/android/App.js.map
#	bundle/android/raw/i18ncache_data_ja.json
#	bundle/android/raw/i18ncache_data_nl.json
#	bundle/android/raw/i18ncache_data_sv.json
#	bundle/android/raw/i18ncache_data_tr.json
#	bundle/ios/App.js
#	bundle/ios/App.js.map
#	bundle/ios/assets/i18n-cache/data/ja.json
#	bundle/ios/assets/i18n-cache/data/nl.json
#	bundle/ios/assets/i18n-cache/data/sv.json
#	bundle/ios/assets/i18n-cache/data/tr.json
#	gutenberg
@mchowning mchowning requested a review from marecar3 June 19, 2020 12:36
@SergioEstevao SergioEstevao changed the base branch from develop to main June 24, 2020 11:01
@SergioEstevao SergioEstevao changed the base branch from main to issue/369_ensure_empty_drafts_are_not_published June 24, 2020 11:02
@SergioEstevao SergioEstevao changed the base branch from issue/369_ensure_empty_drafts_are_not_published to develop June 24, 2020 13:42
@SergioEstevao
Copy link
Contributor Author

Closing this one in favor of #2424 that takes into account the new monorepo structure of the repo.
All the changes that were made in this PR were move to this PR in GB: WordPress/gutenberg#22119

@SergioEstevao SergioEstevao deleted the issue_331/mention_support_trigger_on_keypress branch June 26, 2020 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement Improves a current area of the editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support @-mentions while editing
3 participants