Improve user experience around slash commands in instant comments

changed title from WIP: Improve user experience around slash commands to WIP: Improve user experience around slash commands in instant comments

added 37 commits

85576ca1...9ba8512c - 32 commits from branch master
7220d981 - Add forceRetrieve support for AjaxCache
002ccd17 - Slash command handling improvments and fixes
c88d1041 - Update tests for slash command handling changes
ba43b5f2 - Use AjaxCache for autocompletion, fix command cache issue
c955068a - Add changelog entry

Compare with previous version

changed the description

@mikegreiling Can you do an initial review of this please? since you're already aware of the context.

assigned to @mikegreiling

As discussed in the #fe-architecture channel on Slack, let's rework the cache reset hack by binding a new GfmAutoComplete resetCache() method to a jQuery event set up within setupLifecycle() and then triggering this when the form is submitted.

assigned to @kushalpandya

added 168 commits

c955068a...ec2130be - 161 commits from branch master
4c267076 - Add forceRetrieve support for AjaxCache
613b062f - Slash command handling improvments and fixes
366d7da5 - Update tests for slash command handling changes
04d2a9e4 - Use AjaxCache for autocompletion, fix command cache issue
f80cf0b7 - Add changelog entry
909933b7 - Bind clearCache method to event handler, force retrieve on AjaxCache
50f59991 - Use event trigger to clear commands cache

Compare with previous version

@mikegreiling Thanks for the tip! it works great, ready for review.

marked the checklist item Added for this feature/bug as completed

marked the checklist item Conform by the merge request performance guides as completed

marked the checklist item Conform by the style guides as completed

marked the checklist item Branch has no merge conflicts with master (if it does - rebase it please) as completed

marked the checklist item Squashed related commits together as completed

changed the description

marked the checklist item Changelog entry added, if necessary as completed

assigned to @mikegreiling

When using slash commands, I've noticed the placeholder system note is removed a split second before the final note is added to the DOM causing a stutter like this:

(This was in Safari 10. I haven't tested in Chrome)

assigned to @kushalpandya

resolved all discussions

@mikegreiling

placeholder system note is removed a split second before the final note is added to the DOM causing a stutter

Good catch, I've updated now placeholder is now removed, and here's how it looks now;

It is transitioning fast but you can now notice how placeholder is replaced directly with actual system note instead of a brief pause.

assigned to @mikegreiling

resolved all discussions

Getting close! I've noticed another odd bug.

After submitting a form with only slash commands, the blue "command applied" flash banner appears above the form, but if I then submit another form with only slash commands, that banner then disappears. After this, whenever I submit another slash command the banner never reappears as it should.

Actually I noticed another bug while creating that gif to illustrate the one above. It seems if you use a slash command to add a label, and that label is already assigned, the placeholder note will remain there forever.

(this was after trying to add "discussion" as a label a second time)

I'm not sure how best to tackle this. We don't want the GfmAutoComplete object to be responsible for knowing the issue state or tracking the assigned labels. Could we maybe just have a timer that causes the placeholder note to fade away or display an error message if 10 seconds go by with nothing happening?

Lastly, if you type several slash commands really quickly (which is hard to replicate on local dev because the server responds so quickly), the placeholder notes stack up. However, as soon as the server resolves one of them, all of the placeholders get removed at once.

Perhaps we could treat this situation like you currently do when multiple slash commands are applied in a single form submission. When a placeholder system note already exists, instead of appending another placeholder note we could just modify the existing one to say "Applying multiple commands".

assigned to @kushalpandya

added 176 commits

b3c57ad2...e5226177 - 164 commits from branch master
9e38ddd1 - Add forceRetrieve support for AjaxCache
0fc6adc0 - Slash command handling improvments and fixes
824466b0 - Update tests for slash command handling changes
ae9ddba1 - Use AjaxCache for autocompletion, fix command cache issue
acd8ec78 - Add changelog entry
e5161d85 - Bind clearCache method to event handler, force retrieve on AjaxCache
fbb1d5e2 - Use event trigger to clear commands cache
6da2e3cb - Simplify method binding
4c8c2688 - Prevent abrupt UI change for slash commands, move html escaping to single place
d3978816 - Update tests
38ebf0fa - Fix Flash message and placeholder notes removal bug
f2b4cd26 - Update tests for Flash message removal

Compare with previous version

@mikegreiling Thanks for pointing out bugs around Flash message and placeholder appearance as mentioned in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/11612#note_30816541 & https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/11612#note_30816718. I've fixed those bugs.

As for your suggestion of combining placeholders when there are multiple commands being applied on slow network, I'd suggest to leave it as it is since what happens here is that every time a slash command is posted, we show placeholder while command is actually getting posted on server, and when server responds back, we remove corresponding placeholder.

Now, if user is on slow network, time taken by network request to complete for different requests may be entirely different, and having a combined placeholder will lead to either confusion for user or lot of FE code just to make it appear "smart" (like keeping track of which request completed, and update placeholder to reflect it, etc.), and while showing preview of slash commands is just an addon feature, I'd prefer to keep implementation simple and boring as possible.

assigned to @mikegreiling

changed the description

marked the checklist item All builds are passing as completed

I'm noticing the stutter again when the placeholder note is replaced by the actual system note.

assigned to @kushalpandya

added 430 commits

f2b4cd26...68112433 - 417 commits from branch master
eebc4826 - Add forceRetrieve support for AjaxCache
7bf01c1f - Slash command handling improvments and fixes
0386e10a - Update tests for slash command handling changes
ecd751e5 - Use AjaxCache for autocompletion, fix command cache issue
afc04c64 - Add changelog entry
6f2660f4 - Bind clearCache method to event handler, force retrieve on AjaxCache
d43e4747 - Use event trigger to clear commands cache
c8e5154a - Simplify method binding
c3ffd86f - Prevent abrupt UI change for slash commands, move html escaping to single place
c1273c03 - Update tests
18f9eecb - Fix Flash message and placeholder notes removal bug
534d690c - Update tests for Flash message removal
3ee8a09a - Prevent slash command preview removal immediately

Compare with previous version

@mikegreiling That was due to incorrectly add statement from old behaviour, fixed it.

assigned to @mikegreiling

approved this merge request

great, this looks good to me

awesome work!

retrying a build failure... if it goes through, I'll reassign somebody to merge this, if not I'll assign it back to you to rebase.

unmarked as a Work In Progress

@iamphill I think this one is ready. Can you give it a final review/merge?

assigned to @iamphill

merged

mentioned in commit e90ca0f0

Improve user experience around slash commands in instant comments

What does this MR do?

Are there points in the code the reviewer needs to double check?

Why was this MR needed?

Screenshots

Does this MR meet the acceptance criteria?

What are the relevant issue numbers?

Activity

Admin message

Admin message

Improve user experience around slash commands in instant comments

What does this MR do?

Are there points in the code the reviewer needs to double check?

Why was this MR needed?

Screenshots

Does this MR meet the acceptance criteria?

What are the relevant issue numbers?

Merge request reports

Activity