From 29e77ed25f764122efad2156b48e343be9b94501 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 25 Feb 2014 13:31:44 -0500 Subject: [PATCH] Improve UX of pinning feature Students no longer see a tooltip nor any button affordance (i.e. cursor and color change on hover/focus) on the pinning feature. For moderators, the tooltip is now correct when a thread is pinned, the button is accessible for keyboard-only users and screen readers, and an error dialog now appears if an error occurs in attempting to pin/unpin. JIRA: FOR-192 --- .../view/discussion_content_view_spec.coffee | 2 +- .../discussion_thread_show_view_spec.coffee | 56 ++++++++++++++++++- .../view/discussion_view_spec_helper.coffee | 20 ++++--- .../views/discussion_thread_show_view.coffee | 51 ++++++++++++----- lms/static/sass/_discussion.scss | 13 ++--- .../discussion/_underscore_templates.html | 6 +- 6 files changed, 112 insertions(+), 36 deletions(-) diff --git a/common/static/coffee/spec/discussion/view/discussion_content_view_spec.coffee b/common/static/coffee/spec/discussion/view/discussion_content_view_spec.coffee index d0ff555e58..2a2f3fa7d3 100644 --- a/common/static/coffee/spec/discussion/view/discussion_content_view_spec.coffee +++ b/common/static/coffee/spec/discussion/view/discussion_content_view_spec.coffee @@ -16,7 +16,7 @@ describe "DiscussionContentView", ->

Post body.

Report Misuse
-
+
Pin Thread
""" diff --git a/common/static/coffee/spec/discussion/view/discussion_thread_show_view_spec.coffee b/common/static/coffee/spec/discussion/view/discussion_thread_show_view_spec.coffee index 69e4b231a2..4c6a06e453 100644 --- a/common/static/coffee/spec/discussion/view/discussion_thread_show_view_spec.coffee +++ b/common/static/coffee/spec/discussion/view/discussion_thread_show_view_spec.coffee @@ -6,14 +6,20 @@ describe "DiscussionThreadShowView", -> 0 votes (click to vote) +
+ + Pin Thread +
""" ) + window.$$course_id = "TestOrg/TestCourse/TestRun" + window.user = new DiscussionUser({id: "567", upvoted_ids: []}) @threadData = { id: "dummy", - user_id: "567", - course_id: "TestOrg/TestCourse/TestRun", + user_id: user.id, + course_id: $$course_id, body: "this is a thread", created_at: "2013-04-03T20:08:39Z", abuse_flaggers: [], @@ -22,7 +28,6 @@ describe "DiscussionThreadShowView", -> @thread = new Thread(@threadData) @view = new DiscussionThreadShowView({ model: @thread }) @view.setElement($(".discussion-post")) - window.user = new DiscussionUser({id: "567", upvoted_ids: []}) it "renders the vote correctly", -> DiscussionViewSpecHelper.checkRenderVote(@view, @thread) @@ -38,3 +43,48 @@ describe "DiscussionThreadShowView", -> it "vote button activates on appropriate events", -> DiscussionViewSpecHelper.checkVoteButtonEvents(@view) + + describe "renderPinned", -> + describe "for an unpinned thread", -> + it "renders correctly when pinning is allowed", -> + @thread.updateInfo({ability: {can_openclose: true}}) + @view.renderPinned() + pinElem = @view.$(".discussion-pin") + expect(pinElem.length).toEqual(1) + expect(pinElem).not.toHaveClass("pinned") + expect(pinElem).toHaveClass("notpinned") + expect(pinElem.find(".pin-label")).toHaveHtml("Pin Thread") + expect(pinElem).not.toHaveAttr("data-tooltip") + expect(pinElem).toHaveAttr("aria-pressed", "false") + + # If pinning is not allowed, the pinning UI is not present, so no + # test is needed + + describe "for a pinned thread", -> + beforeEach -> + @thread.set("pinned", true) + + it "renders correctly when unpinning is allowed", -> + @thread.updateInfo({ability: {can_openclose: true}}) + @view.renderPinned() + pinElem = @view.$(".discussion-pin") + expect(pinElem.length).toEqual(1) + expect(pinElem).toHaveClass("pinned") + expect(pinElem).not.toHaveClass("notpinned") + expect(pinElem.find(".pin-label")).toHaveHtml("Pinned, click to unpin") + expect(pinElem).toHaveAttr("data-tooltip", "Click to unpin") + expect(pinElem).toHaveAttr("aria-pressed", "true") + + it "renders correctly when unpinning is not allowed", -> + @view.renderPinned() + pinElem = @view.$(".discussion-pin") + expect(pinElem.length).toEqual(1) + expect(pinElem).toHaveClass("pinned") + expect(pinElem).not.toHaveClass("notpinned") + expect(pinElem.find(".pin-label")).toHaveHtml("Pinned") + expect(pinElem).not.toHaveAttr("data-tooltip") + expect(pinElem).not.toHaveAttr("aria-pressed") + + + it "pinning button activates on appropriate events", -> + DiscussionViewSpecHelper.checkButtonEvents(@view, "togglePin", ".admin-pin") diff --git a/common/static/coffee/spec/discussion/view/discussion_view_spec_helper.coffee b/common/static/coffee/spec/discussion/view/discussion_view_spec_helper.coffee index 042cde5c64..c6c69990fa 100644 --- a/common/static/coffee/spec/discussion/view/discussion_view_spec_helper.coffee +++ b/common/static/coffee/spec/discussion/view/discussion_view_spec_helper.coffee @@ -97,15 +97,19 @@ class @DiscussionViewSpecHelper expect(view.unvote).toHaveBeenCalled() expect(event.preventDefault.callCount).toEqual(2) - @checkVoteButtonEvents = (view) -> - spyOn(view, "toggleVote") - button = view.$el.find(".vote-btn") + @checkButtonEvents = (view, viewFunc, buttonSelector) -> + spy = spyOn(view, viewFunc) + button = view.$el.find(buttonSelector) button.click() - expect(view.toggleVote).toHaveBeenCalled() - view.toggleVote.reset() + expect(spy).toHaveBeenCalled() + spy.reset() button.trigger($.Event("keydown", {which: 13})) - expect(view.toggleVote).not.toHaveBeenCalled() - view.toggleVote.reset() + expect(spy).not.toHaveBeenCalled() + spy.reset() button.trigger($.Event("keydown", {which: 32})) - expect(view.toggleVote).toHaveBeenCalled() + expect(spy).toHaveBeenCalled() + + + @checkVoteButtonEvents = (view) -> + @checkButtonEvents(view, "toggleVote", ".vote-btn") diff --git a/common/static/coffee/src/discussion/views/discussion_thread_show_view.coffee b/common/static/coffee/src/discussion/views/discussion_thread_show_view.coffee index 208c89af77..9be9de7b72 100644 --- a/common/static/coffee/src/discussion/views/discussion_thread_show_view.coffee +++ b/common/static/coffee/src/discussion/views/discussion_thread_show_view.coffee @@ -9,7 +9,10 @@ if Backbone? "click .discussion-flag-abuse": "toggleFlagAbuse" "keydown .discussion-flag-abuse": (event) -> DiscussionUtil.activateOnSpace(event, @toggleFlagAbuse) - "click .admin-pin": "togglePin" + "click .admin-pin": + (event) -> @togglePin(event) + "keydown .admin-pin": + (event) -> DiscussionUtil.activateOnSpace(event, @togglePin) "click .action-follow": "toggleFollowing" "keydown .action-follow": (event) -> DiscussionUtil.activateOnSpace(event, @toggleFollowing) @@ -59,15 +62,36 @@ if Backbone? @$(".discussion-flag-abuse .flag-label").html(gettext("Report Misuse")) renderPinned: => + pinElem = @$(".discussion-pin") + pinLabelElem = pinElem.find(".pin-label") if @model.get("pinned") - @$("[data-role=thread-pin]").addClass("pinned") - @$("[data-role=thread-pin]").removeClass("notpinned") - @$(".discussion-pin .pin-label").html(gettext("Pinned")) + pinElem.addClass("pinned") + pinElem.removeClass("notpinned") + if @model.can("can_openclose") + ### + Translators: The text between start_sr_span and end_span is not shown + in most browsers but will be read by screen readers. + ### + pinLabelElem.html( + interpolate( + gettext("Pinned%(start_sr_span)s, click to unpin%(end_span)s"), + {"start_sr_span": "", "end_span": ""}, + true + ) + ) + pinElem.attr("data-tooltip", gettext("Click to unpin")) + pinElem.attr("aria-pressed", "true") + else + pinLabelElem.html(gettext("Pinned")) + pinElem.removeAttr("data-tooltip") + pinElem.removeAttr("aria-pressed") else - @$("[data-role=thread-pin]").removeClass("pinned") - @$("[data-role=thread-pin]").addClass("notpinned") - @$(".discussion-pin .pin-label").html(gettext("Pin Thread")) - + # If not pinned and not able to pin, pin is not shown + pinElem.removeClass("pinned") + pinElem.addClass("notpinned") + pinLabelElem.html(gettext("Pin Thread")) + pinElem.removeAttr("data-tooltip") + pinElem.attr("aria-pressed", "false") updateModelDetails: => @renderVote() @@ -85,14 +109,14 @@ if Backbone? _delete: (event) -> @trigger "thread:_delete", event - togglePin: (event) -> + togglePin: (event) => event.preventDefault() if @model.get('pinned') @unPin() else @pin() - pin: -> + pin: => url = @model.urlFor("pinThread") DiscussionUtil.safeAjax $elem: @$(".discussion-pin") @@ -102,9 +126,9 @@ if Backbone? if textStatus == 'success' @model.set('pinned', true) error: => - $('.admin-pin').text(gettext("Pinning is not currently available")) + DiscussionUtil.discussionAlert("Sorry", "We had some trouble pinning this thread. Please try again.") - unPin: -> + unPin: => url = @model.urlFor("unPinThread") DiscussionUtil.safeAjax $elem: @$(".discussion-pin") @@ -113,7 +137,8 @@ if Backbone? success: (response, textStatus) => if textStatus == 'success' @model.set('pinned', false) - + error: => + DiscussionUtil.discussionAlert("Sorry", "We had some trouble unpinning this thread. Please try again.") toggleClosed: (event) -> $elem = $(event.target) diff --git a/lms/static/sass/_discussion.scss b/lms/static/sass/_discussion.scss index 43f2cf946c..d3449073f7 100644 --- a/lms/static/sass/_discussion.scss +++ b/lms/static/sass/_discussion.scss @@ -2471,17 +2471,16 @@ body.discussion { float:right; padding-right: 5px; font-style: italic; - cursor: pointer; margin-right: $baseline/2; opacity: 0.8; - span { + &.admin-pin { cursor: pointer; - } - &:hover, &:focus { - @include transition(opacity .2s linear 0s); - opacity: 1.0; + &:hover, &:focus { + @include transition(opacity .2s linear 0s); + opacity: 1.0; + } } } @@ -2520,8 +2519,6 @@ body.discussion { .pinned span { color: $pink; font-style: italic; - //cursor change is here since pins are read-only for inline discussions. - cursor: default; } .notpinned span { diff --git a/lms/templates/discussion/_underscore_templates.html b/lms/templates/discussion/_underscore_templates.html index e592efc9bf..6c0d71adda 100644 --- a/lms/templates/discussion/_underscore_templates.html +++ b/lms/templates/discussion/_underscore_templates.html @@ -62,13 +62,13 @@ % if course and has_permission(user, 'openclose_thread', course.id): -
+
${_("Pin Thread")}
%else: ${"<% if (pinned) { %>"} -
- ${_("Pin Thread")}
+
+ ${_("Pinned")}
${"<% } %>"} % endif