-
Notifications
You must be signed in to change notification settings - Fork 58
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
Issue 331/mention support on @ keypress #2218
Conversation
…rt_trigger_on_keypress # Conflicts: # gutenberg
…rt_trigger_on_keypress # Conflicts: # bundle/android/App.js # bundle/android/App.js.map # bundle/ios/App.js # bundle/ios/App.js.map
…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 } |
There was a problem hiding this comment.
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
…support_trigger_on_keypress
730566e
to
91c1a93
Compare
Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job! |
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? |
…ress # Conflicts: # gutenberg
@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. |
@mchowning @SergioEstevao I had another chance to test on Android in more detail. Here are some notes:
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? |
@thomasbishop The @ button was missing in the toolbar because it was hidden
because of the DEV flag settings.
I will remove the flag and generate another build for your to check.
…On Fri, 12 Jun 2020 at 16:50, Thomas Bishop ***@***.***> wrote:
@mchowning <https://github.com/mchowning> @SergioEstevao
<https://github.com/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 <https://share.icloud.com/photos/0saxnY96Fmp1tInRurfBn1_FA>)
- 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
<https://github.com/SergioEstevao> are you able to make a new test build?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2218 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAE7CUPQFESCUJJS4DVMJLDRWJFFJANCNFSM4MZ654MA>
.
|
…ress # Conflicts: # bundle/android/App.js # bundle/android/App.js.map # bundle/ios/App.js # bundle/ios/App.js.map # gutenberg
@SergioEstevao Tested iOS and it is looking solid — not seeing the aforementioned issues there. 👍 |
👋 @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?
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.
Removed the animation and added the top border back.
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. |
@mchowning Most everything is looking solid, just a couple of things.
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. |
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 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
Closing this one in favor of #2424 that takes into account the new monorepo structure of the repo. |
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:
matt
is addedPR submission checklist:
RELEASE-NOTES.txt
if necessary.