-
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
Add @-mention support #2163
Add @-mention support #2163
Conversation
# Conflicts: # gutenberg
# Conflicts: # gutenberg
# Conflicts: # gutenberg
@mchowning @SergioEstevao Great work thus far! I've only had a chance to take Android for a spin (Sergio is helping to get me an iOS test build, so I'll test iOS asap). Based on that quick spin, here is some initial feedback. (I hope this is the right issue to drop feedback on — if not, I can move to another ticket!) Edit: I'm going through and editing my previous comments after testing with iOS, as some of the issues apply only to Android or iOS, and some to both. I've also added a couple of additional items found during this review. Initial review notesBoth platforms
Android-only
iOS-only
Bonus pointsThis is a bit out of scope, but it would certainly be nice-to-have!
|
iOS-only
|
ok
While I understand this is good to have, it will involve a bit more of a technical challenge in terms that we will need to send back and forward from JS to native the keypresses of @
This is one is even more challenging because for this to work we will need to send all the keypresses between JS and Native, and the performance will suffer because of this.
Do we have this on the web? If not it will need something to be coordinated with our web colleagues.
Easy to do :) |
@SergioEstevao Thanks for the updates!
Fair enough. It would definitely be nice to have, but if it's a lot of work, it could be part of the next iteration.
Same thing as the last one, although I think this is less important than the previous one.
No, we don't have this on the web, but this is how it works on most other products and it feels odd that it's not styled as a link considering that's what the front-end output is. |
Thanks so much @iamthomasbishop ! Since @SergioEstevao has already responded to the iOS and cross-platform issues (and I agree with everything he said), I'll address the Android-specific issues. Android-Only
Can you tell me a bit more about the glitch you're seeing? I'm having trouble recreating it.
I want to do a bit more digging on this, but it's looking like this will be difficult to change. The suggestions are actually appearing in a pop-up window, which the system applies a shadow to. I'm just disguising the fact that it's a popup window by making it full-width. It feels to me like this is not a must-have if it turns out to be particularly difficult, do you agree?
That makes sense! 👍 |
# Conflicts: # gutenberg
Done |
@mchowning I forgot to come back to submit my replies to you on Friday, my bad 🤦
Awesome!
I definitely wouldn’t consider it a blocker, but if we can’t remove the shadow altogether, is there a way we can bump up the z-index of the mention toolbar to be above it? In other words, if the suggestions UI is flush against the toolbar, and the toolbar has a higher z-index, it’d cover the shadow. If that’s not possible, let’s make the UI not-quite full-width — maybe 8 or 16px margin on each side of the suggestions UI? I’m going to play with an updated iOS build now and report back if I have any additional feedback. |
@SergioEstevao I found a tiny thing on iOS, and I’m not sure where it’s coming from. The background color of the suggestions is different depending on the width of the screen/app. For example, on iPad full-screen, the background looks black, whereas on split-view iPad or iPhone, it looks like a secondary surface material (dark gray). Can we force all breakpoints to use the dark gray? Here are some screenshots: Not a blocker, and this might be better left as a refinement to the native part, but I also noticed the suggestions rows going full-width feels a little odd. I wonder if on wider screens we should either make the container of each suggestion narrower or perhaps stack the username and full name in the rows like Github for iOS does — like this: |
As soon as jitpack decides to let the WPAndroid build succeed 😞this will be ready for another review @iamthomasbishop . Here is a summary of where things stand with respect to Android: Done
Not Done on Android
This is working on iOS. At this time I have not made this change on Android, however, because this behavior would be broken currently on Android. The technical issue is that if we think that there is an extra space that the native code might remove we reset the cursor's position. As a result, if the mention ends with a space, the cursor is moved to the beginning of the mention. It seems to me that having the cursor after the just-inserted mention without a space is much less disruptive to the writing flow than having the cursor at the beginning of the mention with a space at the end. I am working on this, but addressing it will probably require touching code that was added to avoid a number of crashes on Android; I want to work on that change separately from this PR since it looks like it will be high risk and should be reviewed in isolation to make sure I don't introduce any regressions. Also, my thinking is that the lack of a space after the mention is probably not a blocking issue.
I'm not clear on what glitch you're seeing here, could you describe it a bit more or record a video? Future ImprovementsThese are things that will require a bit of work to implement (discussed here), and I think we should keep track of them because they would be really good future improvements.
|
ca7bc3b
to
27411aa
Compare
@mchowning Just had a chance to review again.
Agreed — this would be really nice to have, but not adding a space is indeed less disruptive. I don't consider it a pure blocker, but it's close because (imo) it feels like a bug in practice. Addressing in a separate PR is fine.
Sure thing, here's a video. Note: it's happening when I tapped the user from the suggestions list.
Sounds good 👍 |
Thanks for the video of the UI glitch @iamthomasbishop ! That wasn't happening on my test device, but I found an emulator that showed the same issue, and it should be resolved now. Would you mind taking a look at the latest installable build with your device to confirm that it's fixed for you as well? |
# Conflicts: # bundle/android/App.js # bundle/android/App.js.map # bundle/android/raw/i18ncache_data_ar.json # bundle/android/raw/i18ncache_data_bg.json # bundle/android/raw/i18ncache_data_bo.json # bundle/android/raw/i18ncache_data_ca.json # bundle/android/raw/i18ncache_data_cs.json # bundle/android/raw/i18ncache_data_cy.json # bundle/android/raw/i18ncache_data_da.json # bundle/android/raw/i18ncache_data_de.json # bundle/android/raw/i18ncache_data_el.json # bundle/android/raw/i18ncache_data_enau.json # bundle/android/raw/i18ncache_data_enca.json # bundle/android/raw/i18ncache_data_engb.json # bundle/android/raw/i18ncache_data_ennz.json # bundle/android/raw/i18ncache_data_enza.json # bundle/android/raw/i18ncache_data_es.json # bundle/android/raw/i18ncache_data_esar.json # bundle/android/raw/i18ncache_data_escl.json # bundle/android/raw/i18ncache_data_escr.json # bundle/android/raw/i18ncache_data_fa.json # bundle/android/raw/i18ncache_data_fr.json # bundle/android/raw/i18ncache_data_gl.json # bundle/android/raw/i18ncache_data_he.json # bundle/android/raw/i18ncache_data_hr.json # bundle/android/raw/i18ncache_data_hu.json # bundle/android/raw/i18ncache_data_id.json # bundle/android/raw/i18ncache_data_is.json # bundle/android/raw/i18ncache_data_it.json # bundle/android/raw/i18ncache_data_ja.json # bundle/android/raw/i18ncache_data_ka.json # bundle/android/raw/i18ncache_data_ko.json # bundle/android/raw/i18ncache_data_nb.json # bundle/android/raw/i18ncache_data_nl.json # bundle/android/raw/i18ncache_data_nlbe.json # bundle/android/raw/i18ncache_data_pl.json # bundle/android/raw/i18ncache_data_pt.json # bundle/android/raw/i18ncache_data_ptbr.json # bundle/android/raw/i18ncache_data_ro.json # bundle/android/raw/i18ncache_data_ru.json # bundle/android/raw/i18ncache_data_sk.json # bundle/android/raw/i18ncache_data_sq.json # bundle/android/raw/i18ncache_data_sr.json # bundle/android/raw/i18ncache_data_sv.json # bundle/android/raw/i18ncache_data_th.json # bundle/android/raw/i18ncache_data_tr.json # bundle/android/raw/i18ncache_data_uk.json # bundle/android/raw/i18ncache_data_ur.json # bundle/android/raw/i18ncache_data_vi.json # bundle/android/raw/i18ncache_data_zhcn.json # bundle/android/raw/i18ncache_data_zhtw.json # bundle/ios/App.js # bundle/ios/App.js.map # bundle/ios/assets/i18n-cache/data/ar.json # bundle/ios/assets/i18n-cache/data/bg.json # bundle/ios/assets/i18n-cache/data/bo.json # bundle/ios/assets/i18n-cache/data/ca.json # bundle/ios/assets/i18n-cache/data/cs.json # bundle/ios/assets/i18n-cache/data/cy.json # bundle/ios/assets/i18n-cache/data/da.json # bundle/ios/assets/i18n-cache/data/de.json # bundle/ios/assets/i18n-cache/data/el.json # bundle/ios/assets/i18n-cache/data/en-au.json # bundle/ios/assets/i18n-cache/data/en-ca.json # bundle/ios/assets/i18n-cache/data/en-gb.json # bundle/ios/assets/i18n-cache/data/en-nz.json # bundle/ios/assets/i18n-cache/data/en-za.json # bundle/ios/assets/i18n-cache/data/es-ar.json # bundle/ios/assets/i18n-cache/data/es-cl.json # bundle/ios/assets/i18n-cache/data/es-cr.json # bundle/ios/assets/i18n-cache/data/es.json # bundle/ios/assets/i18n-cache/data/fa.json # bundle/ios/assets/i18n-cache/data/fr.json # bundle/ios/assets/i18n-cache/data/gl.json # bundle/ios/assets/i18n-cache/data/he.json # bundle/ios/assets/i18n-cache/data/hr.json # bundle/ios/assets/i18n-cache/data/hu.json # bundle/ios/assets/i18n-cache/data/id.json # bundle/ios/assets/i18n-cache/data/is.json # bundle/ios/assets/i18n-cache/data/it.json # bundle/ios/assets/i18n-cache/data/ja.json # bundle/ios/assets/i18n-cache/data/ka.json # bundle/ios/assets/i18n-cache/data/ko.json # bundle/ios/assets/i18n-cache/data/nb.json # bundle/ios/assets/i18n-cache/data/nl-be.json # bundle/ios/assets/i18n-cache/data/nl.json # bundle/ios/assets/i18n-cache/data/pl.json # bundle/ios/assets/i18n-cache/data/pt-br.json # bundle/ios/assets/i18n-cache/data/pt.json # bundle/ios/assets/i18n-cache/data/ro.json # bundle/ios/assets/i18n-cache/data/ru.json # bundle/ios/assets/i18n-cache/data/sk.json # bundle/ios/assets/i18n-cache/data/sq.json # bundle/ios/assets/i18n-cache/data/sr.json # bundle/ios/assets/i18n-cache/data/sv.json # bundle/ios/assets/i18n-cache/data/th.json # bundle/ios/assets/i18n-cache/data/tr.json # bundle/ios/assets/i18n-cache/data/uk.json # bundle/ios/assets/i18n-cache/data/ur.json # bundle/ios/assets/i18n-cache/data/vi.json # bundle/ios/assets/i18n-cache/data/zh-cn.json # bundle/ios/assets/i18n-cache/data/zh-tw.json # gutenberg
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.
Looks good. Functionality mostly tested via the WPAndroid App and PR instructions. I did check the iOS example app as well which looks good. 🎉
(edit - minor issue that I think is not a blocker, video on gutenberg PR WordPress/gutenberg#21651 (review))
Application application, boolean isDebug, boolean buildGutenbergFromSource, | ||
String postType, boolean isNewPost, String localeString, Bundle translations, | ||
int colorBackground, boolean isDarkMode) { | ||
public void onCreateView(Context initContext, |
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.
I can't see any reason to keep this deprecated, overloaded onCreateView. Introduced 09367d3 but I'm not sure it was ever used for anything, maybe just added to be safe in that initial change I linked. But no real need to "refactor" and remove that in combination with this change if there's a small risk it breaks something that I'm just not imagining, could be removed in a future clean up/refactor PR, so up to you.
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.
I had the exact same thought. I was very tempted to just remove it, but ended up deciding not to lump that change in with these changes.
@@ -194,6 +195,10 @@ extension GutenbergViewController: GutenbergBridgeDelegate { | |||
func gutenbergDidLogUserEvent(_ event: GutenbergUserEvent) { | |||
print("Gutenberg loged user event") | |||
} | |||
|
|||
func gutenbergDidRequestMention(callback: @escaping (Result<String, NSError>) -> Void) { | |||
callback(.success("matt")) |
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.
I'd be in favor of "sergio" here for balance, but I guess the name "matt" get's special treatment around here ;-P
# Conflicts: # bundle/android/App.js # bundle/android/App.js.map # bundle/android/raw/i18ncache_data_ar.json # bundle/android/raw/i18ncache_data_bo.json # bundle/android/raw/i18ncache_data_cs.json # bundle/android/raw/i18ncache_data_fr.json # bundle/android/raw/i18ncache_data_gl.json # bundle/android/raw/i18ncache_data_it.json # bundle/android/raw/i18ncache_data_ko.json # bundle/android/raw/i18ncache_data_nb.json # bundle/android/raw/i18ncache_data_nl.json # bundle/android/raw/i18ncache_data_ro.json # bundle/android/raw/i18ncache_data_ru.json # bundle/android/raw/i18ncache_data_sq.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/ar.json # bundle/ios/assets/i18n-cache/data/bo.json # bundle/ios/assets/i18n-cache/data/cs.json # bundle/ios/assets/i18n-cache/data/fr.json # bundle/ios/assets/i18n-cache/data/gl.json # bundle/ios/assets/i18n-cache/data/it.json # bundle/ios/assets/i18n-cache/data/ko.json # bundle/ios/assets/i18n-cache/data/nb.json # bundle/ios/assets/i18n-cache/data/nl.json # bundle/ios/assets/i18n-cache/data/ro.json # bundle/ios/assets/i18n-cache/data/ru.json # bundle/ios/assets/i18n-cache/data/sq.json # bundle/ios/assets/i18n-cache/data/sv.json # bundle/ios/assets/i18n-cache/data/tr.json # gutenberg
# Conflicts: # bundle/android/App.js # bundle/android/App.js.map # bundle/ios/App.js # bundle/ios/App.js.map # gutenberg
# Conflicts: # bundle/android/App.js # bundle/android/App.js.map # bundle/android/raw/i18ncache_data_de.json # bundle/android/raw/i18ncache_data_pt.json # bundle/android/raw/i18ncache_data_ptbr.json # bundle/android/raw/i18ncache_data_tr.json # bundle/ios/App.js # bundle/ios/App.js.map # bundle/ios/assets/i18n-cache/data/de.json # bundle/ios/assets/i18n-cache/data/pt-br.json # bundle/ios/assets/i18n-cache/data/pt.json # bundle/ios/assets/i18n-cache/data/tr.json # gutenberg
Fixes #331 by adding an
@
-button for inserting mentions for anyRichText
-based block.Gutenberg PR: WordPress/gutenberg#21651
WPAndroid PR: wordpress-mobile/WordPress-Android#11693
WPiOS PR: wordpress-mobile/WordPress-iOS#13942
Testing
PR submission checklist:
RELEASE-NOTES.txt
if necessary.