Skip to content

E
edx-platform
Commit 29e77ed2 by Greg Price
Options

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
parent 0f388920
Showing with 112 additions and 36 deletions
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>
"""
......
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")
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")
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)
......
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 {
......
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
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment