From 05d8eeecd7d8e81c96b64a1092690ace2d112475 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 17 Jun 2014 11:43:26 -0400 Subject: [PATCH] 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 --- .../discussion_thread_list_view_spec.coffee | 27 ++++---- .../views/discussion_thread_list_view.coffee | 59 +++++++++-------- .../test/acceptance/pages/lms/discussion.py | 9 +-- .../test/acceptance/tests/test_discussion.py | 8 +-- lms/static/sass/discussion/_discussion.scss | 65 ------------------- .../sass/discussion/elements/_navigation.scss | 31 +++++++++ .../sass/discussion/utilities/_shame.scss | 13 ++++ .../discussion/_thread_list_template.html | 28 ++++---- .../discussion/_underscore_templates.html | 5 +- 9 files changed, 120 insertions(+), 125 deletions(-) diff --git a/common/static/coffee/spec/discussion/view/discussion_thread_list_view_spec.coffee b/common/static/coffee/spec/discussion/view/discussion_thread_list_view_spec.coffee index 48c941cb13..26160ff8f2 100644 --- a/common/static/coffee/spec/discussion/view/discussion_thread_list_view_spec.coffee +++ b/common/static/coffee/spec/discussion/view/discussion_thread_list_view_spec.coffee @@ -69,13 +69,14 @@ describe "DiscussionThreadListView", -> -
- Sort by: - +
+ + +
@@ -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", -> diff --git a/common/static/coffee/src/discussion/views/discussion_thread_list_view.coffee b/common/static/coffee/src/discussion/views/discussion_thread_list_view.coffee index 5e27aa8f3d..88f5b99067 100644 --- a/common/static/coffee/src/discussion/views/discussion_thread_list_view.coffee +++ b/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("
  • " + gettext("Load more") + "
    • ") @@ -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) -> diff --git a/common/test/acceptance/pages/lms/discussion.py b/common/test/acceptance/pages/lms/discussion.py index 431784716c..cd812a1dbd 100644 --- a/common/test/acceptance/pages/lms/discussion.py +++ b/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): """ diff --git a/common/test/acceptance/tests/test_discussion.py b/common/test/acceptance/tests/test_discussion.py index d7e7be8c29..a71f0f9b31 100644 --- a/common/test/acceptance/tests/test_discussion.py +++ b/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) diff --git a/lms/static/sass/discussion/_discussion.scss b/lms/static/sass/discussion/_discussion.scss index f07f15b0a2..0be8886ac2 100644 --- a/lms/static/sass/discussion/_discussion.scss +++ b/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; diff --git a/lms/static/sass/discussion/elements/_navigation.scss b/lms/static/sass/discussion/elements/_navigation.scss index 7a0b94e2e8..2a1a26df84 100644 --- a/lms/static/sass/discussion/elements/_navigation.scss +++ b/lms/static/sass/discussion/elements/_navigation.scss @@ -1,3 +1,34 @@ +// ------------------- +// 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 // ----------- diff --git a/lms/static/sass/discussion/utilities/_shame.scss b/lms/static/sass/discussion/utilities/_shame.scss index 991cec5df0..0f069889fa 100644 --- a/lms/static/sass/discussion/utilities/_shame.scss +++ b/lms/static/sass/discussion/utilities/_shame.scss @@ -1,3 +1,16 @@ +// -------------------------------- +// 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 diff --git a/lms/templates/discussion/_thread_list_template.html b/lms/templates/discussion/_thread_list_template.html index 0ea9964642..e1c95c8197 100644 --- a/lms/templates/discussion/_thread_list_template.html +++ b/lms/templates/discussion/_thread_list_template.html @@ -26,23 +26,27 @@
    -
    - ${_("Sort by:")} - - +
    %if is_course_cohorted and is_moderator: - ${_("Show:")} - + %for c in cohorts: - + %endfor %endif + + + +
    diff --git a/lms/templates/discussion/_underscore_templates.html b/lms/templates/discussion/_underscore_templates.html index f7b9bbd57a..d6dc58ffca 100644 --- a/lms/templates/discussion/_underscore_templates.html +++ b/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_close': '', - '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}';