Merge pull request #2733 from edx/gprice/forum-pinning-ux

Improve UX of pinning feature
2014-02-26 15:28:21 -05:00
parent 256e13a0af 29e77ed25f
commit 4516f9e99b
6 changed files with 112 additions and 36 deletions
--- 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", ->
                <div class="post-body"><p>Post body.</p></div>
                <div data-tooltip="Report Misuse" data-role="thread-flag" class="discussion-flag-abuse notflagged">
                <i class="icon"></i><span class="flag-label">Report Misuse</span></div>
-                <div data-tooltip="pin this thread" data-role="thread-pin" class="admin-pin discussion-pin notpinned">
+                <div data-tooltip="pin this thread" class="admin-pin discussion-pin notpinned">
                <i class="icon"></i><span class="pin-label">Pin Thread</span></div>
            </div>
            """
--- 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", ->
                <a href="#" class="vote-btn" data-tooltip="vote" role="button" aria-pressed="false">
                    <span class="plus-icon"/><span class="votes-count-number">0</span> <span class="sr">votes (click to vote)</span>
                </a>
+                <div class="admin-pin discussion-pin notpinned" role="button" aria-pressed="false" tabindex="0">
+                    <i class="icon icon-pushpin"></i>
+                    <span class="pin-label">Pin Thread</span>
+                </div>
            </div>
            """
        )

+        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<span class='sr'>, click to unpin</span>")
+                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")
--- 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")
--- 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": "<span class='sr'>", "end_span": "</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)
--- 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 {
--- 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):
-      <div class="admin-pin discussion-pin notpinned" data-role="thread-pin" data-tooltip="${_('pin this thread') | h}">
+      <div class="admin-pin discussion-pin notpinned" role="button" aria-pressed="false" tabindex="0">
      <i class="icon icon-pushpin"></i><span class="pin-label">${_("Pin Thread")}</span></div>

      %else:
      ${"<% if (pinned) { %>"}
-      <div class="discussion-pin notpinned" data-role="thread-pin" data-tooltip="${_('pin this thread') | h}">
-      <i class="icon icon-pushpin"></i><span class="pin-label">${_("Pin Thread")}</span></div>
+      <div class="discussion-pin notpinned">
+      <i class="icon icon-pushpin"></i><span class="pin-label">${_("Pinned")}</span></div>
      ${"<% }  %>"}
      % endif