-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Dismiss suggestions on space keystroke #16601
Conversation
Users expect the suggestions UI to be dismissed when typing a space immediately after a trigger character (e.g. `@` for mentions, `+` for cross-posts). Addresses WordPress/gutenberg#24425 (for iOS)
You can trigger an installable build for these changes by visiting CircleCI here. |
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
WordPress/Classes/ViewRelated/Gutenberg/GutenbergSuggestionsViewController.swift
Show resolved
Hide resolved
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 tested this and it worked really well for me. Nicely done.
Lets 🚢 it
I think this PR is ready to merge. One thing that I found in my testing is that it is kind of hard to add a |
Thanks!
One technique here is to type type-username.movDo you think this is a good user experience or could it be improved? |
Thanks for pointing out that tapping on the editor will get you back into this mode. This is what I saw. RPReplay_Final1622743333.MP4I think one improvement we could make is ( how I expected this would work is) if you type a word and then press space you would get back to the editor. Since that how I would have types it sequentially. Not sure if this has been considered before? |
Thanks for the video and explanation, I think I understand your concern better now. What prevents us from dismissing the suggestion UI when the user presses space after typing a So if a user suggestion is |
// The same applies for cross-posts, with "+ " instead of "@ ". | ||
let dismissSequence = suggestionType.trigger + " " | ||
guard searchWord != dismissSequence else { | ||
onCompletion?(.success("")) |
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.
👋 @guarani ! If I'm understanding this correctly, passing this as a "success" means that we'll send off analytics indicating that the user successfully selected a suggestion, right? If that's what will happen, that doesn't feel right to me. Seems that we should be treating this as a failure case where the user did not select a suggestion (at least for the purposes of the analytics).
I imagine that the reason you're using the success case is because we get the insertion of the "space" for free when we do that, but maybe there are other reasons I'm not thinking of), whereas if we send a "failure" back to JS, then nothing gets inserted (so the space gets dropped). It feels like we have two options here:
- Send back a "success" to javascript (so the space gets inserted), but send a "failure" to the analytics (that's what @SiobhyB has proposed doing on Android); or
- Add a way to send text-to-be-inserted (in this case a single space) back to javascript with a "failure".
Option 2 feels cleaner to me, but it may be over-engineering for the single case where we just want to send a space back since we already get that for free with an "empty" success, so I'm sympathetic to just going with Option 1's simpler approach. If we do that though, I think we may need some comments explaining why we're doing this. Curious to hear what you (and @SiobhyB ) think though.
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.
Thanks for catching this, @mchowning! I completely overlooked the Tracks aspect. I'm going to revert this PR for the time-being while a fix is made.
I think option 1 might work on iOS as well, but will take a look at option 2 as well.
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 created a revert PR here.
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.
Thanks for looping me into the discussion here @mchowning. :)
If there's a possibility of more cases where certain keystrokes serve to both exit the UI and update the editor (like the one proposed in wordpress-mobile/gutenberg-mobile#2943) then I'd definitely prefer to do this in the cleanest, most scalable way possible.
Are there key differences to how the JavaScript side would handle a "success" vs a "failure" message? I'd made the assumption that entering a space was a success as far as the JavaScript/editor was concerned and am not yet clear what sending a failure would look like.
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.
Are there key differences to how the JavaScript side would handle a "success" vs a "failure" message?
Good question. The only difference that I can imagine at this point is the fact that we want to add a space at the end of a successfully inserted mention, whereas we might not want to do that when a mention was not inserted. For example, if we wanted to have a space exit the mention at any point in the mentions UI (which as Paul points out, we probably do not want to do), then we wouldn't want to add an "extra" space on after the "mention text" which already ends in a space.
This is a bit of a made up example though since we don't actually want this behavior, and this is my concern with Option 2: we don't really know what other use case there might be that would need failure-with-text, so it's hard to know how to best design it.
Not that I'm opposed to Option 2--I bias pretty strongly to finding the cleanest approach most often--but in this case I'm not sure whether it's worth much extra effort: your implementation of Option 1 on Android is quite simple and works perfectly. Doing option 1 on iOS probably won't be as simple though, so I'll be curious to see what Paul thinks after he digs in a bit.
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.
Ah, I see, thank you for that extra context!
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 left a comment, wordpress-mobile/gutenberg-mobile#2943. Once that's resolved I think we'll have a better picture of how to handle this.
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 left a comment, wordpress-mobile/gutenberg-mobile#2943. Once that's resolved I think we'll have a better picture of how to handle this.
Now that wordpress-mobile/gutenberg-mobile#2943 (comment) is closed, we don't have another use case for designing a way to pass text back to the React Native world in the suggestions insertion "failure" case. I think Option 1, where we send a success event to React Native and a "failure" to analytics, is a reasonable solution here.
I'll go ahead with this on the iOS side and ping you @SiobhyB once this is ready just to sync up on merging these changes.
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.
Perfect, thank you @guarani!
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.
FIY the iOS PR is up: #16770
Addresses WordPress/gutenberg#24425 (for iOS)
This PR allows users to easily dismiss the suggestion UI by typing a space character after triggering the UI. This means it will now be easier to add a
@
character to a post without triggering the mentions suggestions UI, or add a+
character without triggering the cross-post suggestions UI.I added this change to suggestions in the block editor, but didn't apply it to other parts of the app (e.g. comments, notification replies, etc) because that felt too far out-of-scope here. It would also make the testing steps much longer.
To test:
@
character dismisses the mentions suggestions list+
character dismisses the cross-positing suggestions listRegression Notes
This change could conceivably break suggestions (both mentions and cross-posting).
The above manual tests.
This change affects the UI so a UI test would be best, but we only add UI tests to core flows.
PR submission checklist:
RELEASE-NOTES.txt
if necessary.