Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Dismiss suggestions on space keystroke #16601
Changes from all commits
831307a
2380676
8f511be
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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:
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.
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.
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