fix ordering when adding a discussion reply; scroll to new threaded reply

For some reason, we were placing discussion replies at the top of the
reply list when adding a reply to a non-threaded discussion. For
example, imagine the following flat discussion:

 Topic
   1st reply
     1st subreply
     2nd subreply
     + "Add Reply"

If you want to add a subreply to "1st reply", you start typing in the
"Add Reply" field at the bottom of the list. Then when you submit,
here's where it shows up:

 Topic
   1st reply
     3rd subreply <--- new reply you just made
     1st subreply
     2nd subreply

Uhh... Then when you reload, it will show up at the bottom, like this:

 Topic
   1st reply
     1st subreply
     2nd subreply
     3rd subreply <--- your new reply

This does not happen on threaded discussions - with those, your reply is
added to the bottom of the reply list like it should.

This doesn't make any sense for 2 reasons:

 1) When you reload, the entry shows at the bottom of the list. So it's
    being put where it won't show up in the real view. This is super
    confusing for users replying to discussions.

 2) With non-threaded discussions you reply at the bottom of the
    reply chain. This would put your reply at the top of the reply
    so you often wouldn't even see it at all! This results in lots
    of people responding multiple times, thinking their reply is
    not working.

I traced down the history of this code. In 2013, it looks like the logic
was inverted - it used to do this behavior on threaded discussions only.
This actually makes slightly more sense, in that it negates point #2
above - when responding to a post, you hit "Reply" directly below the
post (no matter how many replies there are), and then it would put your
reply there right where you can see it. The ordering was still wrong -
your reply would be put at the bottom on a reload. However, at least it
didn't look like your reply disappeared.

The behavior we have now is the worst of both worlds - replies don't
show up where you made them, and you don't see them when you make them.
This change makes it so at least the reply goes where it should go. It
also adds a "scroll into view" when adding replies to threaded
discussions. Since you compose those at the TOP of the children list,
and your reply (correctly) goes to the BOTTOM of the children list,
oftentimes you don't see it. Now we scroll your newly added comment into
view.

test plan:
 * Make a non-threaded discussion.
 * Add a reply to it
 * Add a reply to that reply
 * Keep adding replies to the first reply, and see how they correctly
   show up at the bottom of the list.

 * Make a threaded discussion.
 * Add a reply to it
 * Add a bunch of replies (longer than the page) to the reply from above
 * Verify that when adding new replies, the window scrolls to the newly
   added reply.

Change-Id: I5c0da85962352b5453b09526413b3c68f0a86695
Reviewed-on: https://gerrit.instructure.com/63435
Reviewed-by: John Corrigan <jcorrigan@instructure.com>
Reviewed-by: Ethan Vizitei <evizitei@instructure.com>
QA-Review: Michael Hargiss <mhargiss@instructure.com>
Tested-by: Jenkins
Product-Review: Zach Wily <zach@instructure.com>
This commit is contained in:
Zach Wily 2015-09-16 11:44:34 -06:00 committed by Zach Wily
parent aba0de7b96
commit 9eba534ba3
1 changed files with 7 additions and 6 deletions

View File

@ -5,7 +5,8 @@ define [
'Backbone'
'jst/discussions/EntryCollectionView'
'jst/discussions/entryStats'
'compiled/views/DiscussionTopic/EntryView'
'compiled/views/DiscussionTopic/EntryView',
'compiled/jquery/scrollIntoView'
], (I18n, $, walk, {View}, template, entryStatsTemplate, EntryView) ->
class EntryCollectionView extends View
@ -29,6 +30,8 @@ define [
template: template
$window: $ window
els: '.discussion-entries': 'list'
initialize: ->
@ -80,12 +83,11 @@ define [
addNewView: (view) ->
view.model.set 'new', false
if !@options.threaded and !@options.root
@list.prepend view.el
else
@list.append view.el
@nestEntries()
if not @options.root
@$window.scrollTo view.$el, 200
view.$el.hide()
setTimeout =>
view.$el.fadeIn()
@ -144,4 +146,3 @@ define [
@renderNextLink()
filter: @::afterRender