Improve forum sorting and cohort filtering UX

Thread entries now show an activity count that includes the original
post, and only one of the vote count and activity count is displayed
based on the user's selected sort. The sorting and filtering options now
both use a select and are somewhat more verbose, and the visual styling
of the sort/filter bar is updated.

Originally reviewed in #4165 with a bugfix in #4211

common/static/coffee/spec/discussion/view/discussion_thread_list_view_spec.coffee

@@ -69,13 +69,14 @@ describe "DiscussionThreadListView", ->
                     </form>
                 </div>
             </div>
-            <div class="sort-bar">
-              <span class="sort-label" id="sort-label">Sort by:</span>
-              <ul role="radiogroup" aria-labelledby="sort-label">
-                  <li><a href="#" role="radio" aria-checked="false" data-sort="date">date</a></li>
-                  <li><a href="#" role="radio" aria-checked="false" data-sort="votes">votes</a></li>
-                  <li><a href="#" role="radio" aria-checked="false" data-sort="comments">comments</a></li>
-              </ul>
+            <div class="forum-nav-refine-bar">
+                <span class="forum-nav-sort">
+                    <select class="forum-nav-sort-control">
+                        <option value="date">by recent activity</option>
+                        <option value="comments">by most activity</option>
+                        <option value="votes">by most votes</option>
+                    </select>
+                </span>
             </div>
             <div class="search-alerts"></div>
             <ul class="forum-nav-thread-list"></ul>
@@ -149,7 +150,7 @@ describe "DiscussionThreadListView", ->
       expect(view.$el.find(".forum-nav-thread:nth-child(1) .forum-nav-thread-title").text()).toEqual(sort_order[0])
       expect(view.$el.find(".forum-nav-thread:nth-child(2) .forum-nav-thread-title").text()).toEqual(sort_order[1])
       expect(view.$el.find(".forum-nav-thread:nth-child(3) .forum-nav-thread-title").text()).toEqual(sort_order[2])
-      expect(view.$el.find(".sort-bar a.active").text()).toEqual(type)
+      expect(view.$el.find(".forum-nav-sort-control").val()).toEqual(type)
 
     describe "thread rendering should be correct", ->
         checkRender = (threads, type, sort_order) ->
@@ -157,6 +158,8 @@ describe "DiscussionThreadListView", ->
             view = makeView(discussion)
             view.render()
             checkThreadsOrdering(view, sort_order, type)
+            expect(view.$el.find(".forum-nav-thread-comments-count:visible").length).toEqual(if type == "votes" then 0 else 3)
+            expect(view.$el.find(".forum-nav-thread-votes-count:visible").length).toEqual(if type == "votes" then 3 else 0)
 
         it "with sort preference date", ->
             checkRender(@threads, "date", [ "Thread1", "Thread2", "Thread3"])
@@ -167,12 +170,13 @@ describe "DiscussionThreadListView", ->
         it "with sort preference comments", ->
             checkRender(@threads, "comments", [ "Thread3", "Thread2", "Thread1"])
 
-    describe "Sort click should be correct", ->
+    describe "Sort change should be correct", ->
       changeSorting = (threads, selected_type, new_type, sort_order) ->
         discussion = new Discussion(_.map(threads, (thread) -> new Thread(thread)), {pages: 1, sort: selected_type})
         view = makeView(discussion)
         view.render()
-        expect(view.$el.find(".sort-bar a.active").text()).toEqual(selected_type)
+        sortControl = view.$el.find(".forum-nav-sort-control")
+        expect(sortControl.val()).toEqual(selected_type)
         sorted_threads = []
         if new_type == 'date'
           sorted_threads = [threads[0], threads[1], threads[2]]
@@ -186,9 +190,8 @@ describe "DiscussionThreadListView", ->
           )
           {always: ->}
         )
-        view.$el.find(".sort-bar a[data-sort='"+new_type+"']").click()
+        sortControl.val(new_type).change()
         expect($.ajax).toHaveBeenCalled()
-        expect(view.sortBy).toEqual(new_type)
         checkThreadsOrdering(view, sort_order, new_type)
 
       it "with sort preference date", ->

common/static/coffee/src/discussion/views/discussion_thread_list_view.coffee

@@ -6,18 +6,17 @@ if Backbone?
       "click .browse": "toggleTopicDrop"
       "keydown .post-search-field": "performSearch"
       "focus .post-search-field": "showSearch"
-      "click .sort-bar a": "sortThreads"
+      "change .forum-nav-sort-control": "sortThreads"
       "click .browse-topic-drop-menu": "filterTopic"
       "click .browse-topic-drop-search-input": "ignoreClick"
       "click .forum-nav-thread-link": "threadSelected"
       "click .forum-nav-load-more-link": "loadMorePages"
-      "change .cohort-options": "chooseCohort"
+      "change .forum-nav-filter-cohort-control": "chooseCohort"
       'keyup .browse-topic-drop-search-input': DiscussionFilter.filterDrop
 
     initialize: ->
       @displayedCollection = new Discussion(@collection.models, pages: @collection.pages)
       @collection.on "change", @reloadDisplayedCollection
-      @sortBy = "date"
       @discussionIds=""
       @collection.on "reset", (discussion) =>
         board = $(".current-board").html()
@@ -30,7 +29,6 @@ if Backbone?
         #   @filterTopic($.Event("filter", {'target': target[0]}))
       @collection.on "add", @addAndSelectThread
       @sidebar_padding = 10
-      @sidebar_header_height = 87
       @boardName
       @template = _.template($("#thread-list-template").html())
       @current_search = ""
@@ -71,6 +69,7 @@ if Backbone?
       current_el = @$(".forum-nav-thread[data-id=#{thread_id}]")
       active = current_el.has(".forum-nav-thread-link.is-active").length != 0
       current_el.replaceWith(content)
+      @showMetadataAccordingToSort()
       if active
         @setActiveThread(thread_id)
 
@@ -88,7 +87,7 @@ if Backbone?
       scrollTop = $(window).scrollTop();
       windowHeight = $(window).height();
 
-      discussionBody = $(".discussion-article")
+      discussionBody = $(".discussion-column")
       discussionsBodyTop = if discussionBody[0] then discussionBody.offset().top
       discussionsBodyBottom = discussionsBodyTop + discussionBody.outerHeight()
 
@@ -111,7 +110,9 @@ if Backbone?
       sidebarHeight = Math.min(sidebarHeight + 1, discussionBody.outerHeight())
       sidebar.css 'height', sidebarHeight
 
-      @$('.forum-nav-thread-list').css('height', (sidebarHeight - @sidebar_header_height - 4) + 'px')
+      browseSearchHeight = @$(".browse-search").outerHeight()
+      refineBarHeight = @$(".forum-nav-refine-bar").outerHeight()
+      @$('.forum-nav-thread-list').css('height', (sidebarHeight - browseSearchHeight - refineBarHeight - 2) + 'px')
 
 
     # Because we want the behavior that when the body is clicked the menu is
@@ -123,6 +124,7 @@ if Backbone?
     render: ->
       @timer = 0
       @$el.html(@template())
+      @$(".forum-nav-sort-control").val(@collection.sort_preference)
 
       $(window).bind "load", @updateSidebar
       $(window).bind "scroll", @updateSidebar
@@ -131,9 +133,6 @@ if Backbone?
       @displayedCollection.on "reset", @renderThreads
       @displayedCollection.on "thread:remove", @renderThreads
       @renderThreads()
-      sort_element = @$('.sort-bar a[data-sort="' + this.collection.sort_preference + '"]')
-      sort_element.attr('aria-checked',true)
-      sort_element.addClass('active')
       @
 
     renderThreads: =>
@@ -144,10 +143,24 @@ if Backbone?
         rendered.append content
 
       @$(".forum-nav-thread-list").html(rendered.html())
+      @showMetadataAccordingToSort()
+
       @renderMorePages()
       @updateSidebar()
       @trigger "threads:rendered"
 
+    showMetadataAccordingToSort: () =>
+      # Ensure that threads display metadata appropriate for the current sort
+      voteCounts = @$(".forum-nav-thread-votes-count")
+      commentCounts = @$(".forum-nav-thread-comments-count")
+      voteCounts.hide()
+      commentCounts.hide()
+      switch @$(".forum-nav-sort-control").val()
+        when "date", "comments"
+          commentCounts.show()
+        when "votes"
+          voteCounts.show()
+
     renderMorePages: ->
       if @displayedCollection.hasMorePages()
         @$(".forum-nav-thread-list").append("<li class='forum-nav-load-more'><a href='#' class='forum-nav-load-more-link'>" + gettext("Load more") + "</a></li>")
@@ -197,17 +210,17 @@ if Backbone?
         @renderThreads()
         DiscussionUtil.discussionAlert(gettext("Sorry"), gettext("We had some trouble loading more threads. Please try again."))
 
-      @collection.retrieveAnotherPage(@mode, options, {sort_key: @sortBy}, error)
+      @collection.retrieveAnotherPage(@mode, options, {sort_key: @$(".forum-nav-sort-control").val()}, error)
 
     renderThread: (thread) =>
       content = $(_.template($("#thread-list-item-template").html())(thread.toJSON()))
-      unreadCount = thread.get('unread_comments_count')
+      unreadCount = thread.get('unread_comments_count') + (if thread.get("read") then 0 else 1)
       if unreadCount > 0
         content.find('.forum-nav-thread-comments-count').attr(
           "data-tooltip",
           interpolate(
             ngettext('%(unread_count)s new comment', '%(unread_count)s new comments', unreadCount),
-            {unread_count: thread.get('unread_comments_count')},
+            {unread_count: unreadCount},
             true
           )
         )
@@ -340,26 +353,26 @@ if Backbone?
         if discussionId == "#all"
           @discussionIds = ""
           @$(".post-search-field").val("")
-          @$('.cohort').show()                    
+          @$('.forum-nav-filter-cohort').show()
           @retrieveAllThreads()
         else if discussionId == "#flagged"
           @discussionIds = ""
           @$(".post-search-field").val("")
-          @$('.cohort').hide() 
+          @$('.forum-nav-filter-cohort').hide()
           @retrieveFlaggedThreads() 
         else if discussionId == "#following"
           @retrieveFollowed(event)
-          @$('.cohort').hide()
+          @$('.forum-nav-filter-cohort').hide()
         else
           discussionIds = _.map item.find(".board-name[data-discussion_id]"), (board) -> $(board).data("discussion_id").id
           
           if $(event.target).attr('cohorted') == "True"
-            @retrieveDiscussions(discussionIds, "function(){$('.cohort').show();}")
+            @retrieveDiscussions(discussionIds, "function(){$('.forum-nav-filter-cohort').show();}")
           else
-            @retrieveDiscussions(discussionIds, "function(){$('.cohort').hide();}")
+            @retrieveDiscussions(discussionIds, "function(){$('.forum-nav-filter-cohort').hide();}")
     
     chooseCohort: (event) ->
-      @group_id = @$('.cohort-options :selected').val()
+      @group_id = @$('.forum-nav-filter-cohort-control :selected').val()
       @collection.current_page = 0
       @collection.reset()
       @loadMorePages(event)
@@ -400,14 +413,8 @@ if Backbone?
       @loadMorePages(event)
 
     sortThreads: (event) ->
-      activeSort = @$(".sort-bar a.active")
-      activeSort.removeClass("active")
-      activeSort.attr("aria-checked", "false")
-      newSort = $(event.target)
-      newSort.addClass("active")
-      newSort.attr("aria-checked", "true")
-      @sortBy = newSort.data("sort")
-      @displayedCollection.setSortComparator(@sortBy)
+      @displayedCollection.setSortComparator(@$(".forum-nav-sort-control").val())
+
       @retrieveFirstPage(event)
 
     performSearch: (event) ->

common/test/acceptance/pages/lms/discussion.py

@@ -179,19 +179,20 @@ class DiscussionSortPreferencePage(CoursePage):
         """
         Return true if the browser is on the right page else false.
         """
-        return self.q(css="body.discussion .sort-bar").present
+        return self.q(css="body.discussion .forum-nav-sort-control").present
 
-    def get_selected_sort_preference_text(self):
+    def get_selected_sort_preference(self):
         """
         Return the text of option that is selected for sorting.
         """
-        return self.q(css="body.discussion .sort-bar a.active").text[0].lower()
+        options = self.q(css="body.discussion .forum-nav-sort-control option")
+        return options.filter(lambda el: el.is_selected())[0].get_attribute("value")
 
     def change_sort_preference(self, sort_by):
         """
         Change the option of sorting by clicking on new option.
         """
-        self.q(css="body.discussion .sort-bar a[data-sort='{0}']".format(sort_by)).click()
+        self.q(css="body.discussion .forum-nav-sort-control option[value='{0}']".format(sort_by)).click()
 
     def refresh_page(self):
         """

common/test/acceptance/tests/test_discussion.py

@@ -509,7 +509,7 @@ class DiscussionSortPreferenceTest(UniqueCourseTest):
         """
         Test to check the default sorting preference of user. (Default = date )
         """
-        selected_sort = self.sort_page.get_selected_sort_preference_text()
+        selected_sort = self.sort_page.get_selected_sort_preference()
         self.assertEqual(selected_sort, "date")
 
     def test_change_sort_preference(self):
@@ -520,7 +520,7 @@ class DiscussionSortPreferenceTest(UniqueCourseTest):
         for sort_type in ["votes", "comments", "date"]:
             self.assertNotEqual(selected_sort, sort_type)
             self.sort_page.change_sort_preference(sort_type)
-            selected_sort = self.sort_page.get_selected_sort_preference_text()
+            selected_sort = self.sort_page.get_selected_sort_preference()
             self.assertEqual(selected_sort, sort_type)
 
     def test_last_preference_saved(self):
@@ -531,8 +531,8 @@ class DiscussionSortPreferenceTest(UniqueCourseTest):
         for sort_type in ["votes", "comments", "date"]:
             self.assertNotEqual(selected_sort, sort_type)
             self.sort_page.change_sort_preference(sort_type)
-            selected_sort = self.sort_page.get_selected_sort_preference_text()
+            selected_sort = self.sort_page.get_selected_sort_preference()
             self.assertEqual(selected_sort, sort_type)
             self.sort_page.refresh_page()
-            selected_sort = self.sort_page.get_selected_sort_preference_text()
+            selected_sort = self.sort_page.get_selected_sort_preference()
             self.assertEqual(selected_sort, sort_type)

lms/static/sass/discussion/_discussion.scss

@@ -888,71 +888,6 @@ body.discussion {
     }
   }
 
-  .sort-bar {
-    height: auto;
-    min-height: 27px;
-    border-bottom: 1px solid #a3a3a3;
-    @include linear-gradient(top, rgba(255, 255, 255, .3), rgba(255, 255, 255, 0));
-    background-color: #aeaeae;
-    box-shadow: 0 1px 0 rgba(255, 255, 255, .2) inset;
-
-    span,
-    a {
-      font-size: 9px;
-      font-weight: bold;
-      line-height: 25px;
-      color: #333;
-      text-transform: uppercase;
-      text-shadow: 0 1px 0 rgba(255, 255, 255, .4);
-    }
-
-    .sort-label {
-      display: block;
-      float: left;
-      margin: 0 $baseline/2;
-    }
-
-    li {
-      float: left;
-      margin: 4px 4px 0 0;
-    }
-
-    a {
-      display: block;
-      height: 18px;
-      padding: 0 9px;
-      border-radius: 19px;
-      color: #333;
-      line-height: 17px;
-
-      &:hover, &:focus {
-        @include linear-gradient(top, rgba(255, 255, 255, .4), rgba(255, 255, 255, .2));
-        color: #333;
-      }
-
-      &.active {
-        @include linear-gradient(top, rgba(0, 0, 0, .3), rgba(0, 0, 0, 0));
-        background-color: #999;
-        color: $white;
-        text-shadow: 0 -1px 0 rgba(0, 0, 0, .3);
-        box-shadow: 0 1px 0 rgba(255, 255, 255, 0.2), 0 1px 1px rgba(0, 0, 0, .2) inset;
-      }
-
-    }
-    .group-filter-label {
-      width: 40px;
-      margin-left: $baseline/2;
-    }
-
-    .group-filter-select {
-      margin: 5px 0px 5px 5px;
-      width: 80px;
-      font-size:10px;
-      background: transparent;
-      border-color: #ccc;
-    }
-  }
-
   .bottom-post-status {
     padding: 30px;
     font-size: 20px;

lms/static/sass/discussion/elements/_navigation.scss

+// -------------------
+// Sort and filter bar
+// -------------------
+.forum-nav-refine-bar {
+  @include clearfix();
+  @include font-size(11);
+  border-bottom: 1px solid $gray-l3;
+  background-color: $gray-l5;
+  padding: ($baseline/4) ($baseline/2);
+  color: $black;
+}
+
+%forum-nav-select {
+  border: none;
+  max-width: 100%;
+  background-color: transparent;
+  font: inherit;
+}
+
+.forum-nav-filter-cohort-control {
+  @extend %forum-nav-select;
+}
+
+.forum-nav-sort {
+  float: right;
+}
+
+.forum-nav-sort-control {
+  @extend %forum-nav-select;
+}
+
 // -----------
 // Thread list
 // -----------

lms/static/sass/discussion/utilities/_shame.scss

+// --------------------------------
+// navigation - sort and filter bar
+// --------------------------------
+
+// Override global span rules
+.forum-nav-sort-label {
+  color: inherit;
+}
+
+// --------------------------------
+// navigation - thread list
+// --------------------------------
+
 // The sidebar class does a lot of things that we don't want in the thread list;
 // the following rules contain styling that is necessary and would otherwise
 // reside in elements/_navigation.scss if the sidebar styling did not make the

lms/templates/discussion/_thread_list_template.html

@@ -26,23 +26,27 @@
             </form>
         </div>
     </div>
-    <div class="sort-bar">
-        <span class="sort-label" id="sort-label">${_("Sort by:")}</span>
-        <ul role="radiogroup" aria-labelledby="sort-label">
-            <li><a href="#" role="radio" aria-checked="false" data-sort="date">${_("date")}</a></li>
-            <li><a href="#" role="radio" aria-checked="false" data-sort="votes">${_("votes")}</a></li>
-            <li><a href="#" role="radio" aria-checked="false" data-sort="comments">${_("comments")}</a></li>
-        </ul>
-
+    <div class="forum-nav-refine-bar">
         %if is_course_cohorted and is_moderator:
-        <span class="group-filter-label cohort">${_("Show:")}</span>
-            <select class="group-filter-select cohort-options cohort">
-                <option value="all">${_("View All")}</option>
+        <span class="forum-nav-filter-cohort">
+            <select class="forum-nav-filter-cohort-control">
+                <option value="all">${_("View all cohorts")}</option>
                     %for c in cohorts:
-                        <option value="${c['id']}">${_("View as {name}").format(name=c['name'])}</option>
+                        <option value="${c['id']}">${_("View as {cohort_name}").format(cohort_name=c['name'])}</option>
                     %endfor
             </select>
         %endif
+
+        <span class="forum-nav-sort">
+            <select class="forum-nav-sort-control">
+                ## Translators: This is a menu option for sorting forum threads
+                <option value="date">${_("by recent activity")}</option>
+                ## Translators: This is a menu option for sorting forum threads
+                <option value="comments">${_("by most activity")}</option>
+                ## Translators: This is a menu option for sorting forum threads
+                <option value="votes">${_("by most votes")}</option>
+            </select>
+        </span>
     </div>
     <div class="search-alerts"></div>
     <ul class="forum-nav-thread-list"></ul>

lms/templates/discussion/_underscore_templates.html

@@ -260,11 +260,12 @@
         <% 
         js_block =  u"""
         var fmt;
+        // Counts in data do not include the post itself, but the UI should
         var data = {{
             'span_sr_open': '<span class=\"sr\">',
             'span_close': '</span>',
-            'unread_comments_count': unread_comments_count,
-            'comments_count': comments_count
+            'unread_comments_count': unread_comments_count + (read ? 0 : 1),
+            'comments_count': comments_count + 1
             }};
         if (unread_comments_count > 0) {{
             fmt = '{markup_with_unread}';