-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Add '@' button for inserting mentions on mobile #21651
Conversation
Size Change: 0 B Total Size: 1.11 MB ℹ️ View Unchanged
|
On Android we are discarding selections when we think they might get stripped by Aztec, so adding the space to the ends results in the selection values getting discarded.
# Conflicts: # packages/components/src/index.native.js
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.
Changes here look good! 👍
I did find one very, very small issue. I think it could be logged and saved for later, but I'll leave a video here in case you would like to address now. Basically hitting back button after adding a mention, only when there is not a space before the mention will leave the cursor in the wrong location. See gif. If there is a space before the mention (which seems like it will be the more popular case), the back button leaves the cursor in the correct spot.
|
||
onSelectionChange( index, index ); | ||
onSelectionChange( this.selectionStart, this.selectionEnd ); |
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.
Curious if this is related to mentions or if there is a specific case that this update is addressing? @SergioEstevao
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.
When doing insert of the mentions we found a crash. It was related to the edge case where those indexes were set to NaN and then send to iOS native view.
That didn't like it and make it crash, I think this new code brought this edge case up and so I did this fix.
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.
Makes sense, thanks for the explanation!
Description
This PR adds an
@
button for inserting mentions on mobile (wordpress-mobile/gutenberg-mobile#331).How has this been tested?
RichText
-based block.@
button on the far right of the format toolbarChecklist: