Skip to content

E
edx-platform
Commit 187783be by Brian Jacobel
Options

Requested UI changes - borderless, all posts is no longer highlighted 

Fix read only support for team discussions

Fix error messages to use "posts" not "threads"

More small UI tweaks, padding and alignment

Add a11y tests for all inline discussion states

Improve discussion error messages
parent 95af1843
Showing with 80 additions and 58 deletions
common/static/common/js/discussion/utils.js
...@@ -184,10 +184,8 @@ ...@@ -184,10 +184,8 @@
if (!params.error) { if (!params.error) {
params.error = function() { params.error = function() {
self.discussionAlert( self.discussionAlert(
gettext('Sorry'), gettext('Error'),
gettext( gettext('Your request could not be processed. Refresh the page and try again.')
'We had some trouble processing your request. Please ensure you have copied any ' +
'unsaved work and then reload the page.')
); );
}; };
} }
...@@ -223,7 +221,7 @@ ...@@ -223,7 +221,7 @@
self = this; self = this;
if (errorMsg) { if (errorMsg) {
safeAjaxParams.error = function() { safeAjaxParams.error = function() {
return self.discussionAlert(gettext('Sorry'), errorMsg); return self.discussionAlert(gettext('Error'), errorMsg);
}; };
} }
undo = _.pick(model.attributes, _.keys(updates)); undo = _.pick(model.attributes, _.keys(updates));
...@@ -276,7 +274,7 @@ ...@@ -276,7 +274,7 @@
} }
} }
} else { } else {
$errorItem = makeErrorElem('We had some trouble processing your request. Please try again.', 0); $errorItem = makeErrorElem('Your request could not be processed. Refresh the page and try again.', 0); // eslint-disable-line max-len
edx.HtmlUtils.append(errorsField, $errorItem); edx.HtmlUtils.append(errorsField, $errorItem);
} }
......
common/static/common/js/discussion/views/discussion_inline_view.js
...@@ -26,6 +26,7 @@ ...@@ -26,6 +26,7 @@
var match; var match;
this.$el = options.el; this.$el = options.el;
this.readOnly = options.readOnly;
this.showByDefault = options.showByDefault || false; this.showByDefault = options.showByDefault || false;
this.toggleDiscussionBtn = this.$('.discussion-show'); this.toggleDiscussionBtn = this.$('.discussion-show');
this.listenTo(this.model, 'change', this.render); this.listenTo(this.model, 'change', this.render);
...@@ -144,6 +145,7 @@ ...@@ -144,6 +145,7 @@
course_settings: this.course_settings course_settings: this.course_settings
}); });
this.threadView.render(); this.threadView.render();
this.listenTo(this.threadView.showView, 'thread:_delete', this.navigateToAllPosts);
this.threadListView.$el.addClass('is-hidden'); this.threadListView.$el.addClass('is-hidden');
this.$('.inline-thread').removeClass('is-hidden'); this.$('.inline-thread').removeClass('is-hidden');
}, },
...@@ -179,8 +181,8 @@ ...@@ -179,8 +181,8 @@
this.loadDiscussions(this.$el, function() { this.loadDiscussions(this.$el, function() {
self.hideDiscussion(); self.hideDiscussion();
DiscussionUtil.discussionAlert( DiscussionUtil.discussionAlert(
gettext('Sorry'), gettext('Error'),
gettext('We had some trouble loading the discussion. Please try again.') gettext('This discussion could not be loaded. Refresh the page to try again.')
); );
}); });
} }
......
common/static/common/js/discussion/views/discussion_thread_list_view.js
...@@ -309,7 +309,8 @@ ...@@ -309,7 +309,8 @@
error = function() { error = function() {
self.renderThreads(); self.renderThreads();
DiscussionUtil.discussionAlert( DiscussionUtil.discussionAlert(
gettext('Sorry'), gettext('We had some trouble loading more threads. Please try again.') gettext('Error'),
gettext('Additional posts could not be loaded. Refresh the page to try again.')
); );
}; };
return this.collection.retrieveAnotherPage(this.mode, options, { return this.collection.retrieveAnotherPage(this.mode, options, {
...@@ -478,7 +479,7 @@ ...@@ -478,7 +479,7 @@
element, element,
edx.HtmlUtils.joinHtml( edx.HtmlUtils.joinHtml(
edx.HtmlUtils.HTML("<li class='forum-nav-load-more'>"), edx.HtmlUtils.HTML("<li class='forum-nav-load-more'>"),
self.getLoadingContent(gettext('Loading thread list')), self.getLoadingContent(gettext('Loading posts list')),
edx.HtmlUtils.HTML('</li>') edx.HtmlUtils.HTML('</li>')
) )
); );
...@@ -515,7 +516,7 @@ ...@@ -515,7 +516,7 @@
); );
self.addSearchAlert(message); self.addSearchAlert(message);
} else if (response.discussion_data.length === 0) { } else if (response.discussion_data.length === 0) {
self.addSearchAlert(gettext('No threads matched your query.')); self.addSearchAlert(gettext('No posts matched your query.'));
} }
self.displayedCollection.reset(self.collection.models); self.displayedCollection.reset(self.collection.models);
if (text) { if (text) {
......
common/static/common/js/discussion/views/discussion_thread_view.js
...@@ -261,18 +261,18 @@ ...@@ -261,18 +261,18 @@
} }
if (xhr.status === 404) { if (xhr.status === 404) {
DiscussionUtil.discussionAlert( DiscussionUtil.discussionAlert(
gettext('Sorry'), gettext('Error'),
gettext('The thread you selected has been deleted. Please select another thread.') gettext('The post you selected has been deleted.')
); );
} else if (firstLoad) { } else if (firstLoad) {
DiscussionUtil.discussionAlert( DiscussionUtil.discussionAlert(
gettext('Sorry'), gettext('Error'),
gettext('We had some trouble loading responses. Please reload the page.') gettext('Responses could not be loaded. Refresh the page to try again.')
); );
} else { } else {
DiscussionUtil.discussionAlert( DiscussionUtil.discussionAlert(
gettext('Sorry'), gettext('Error'),
gettext('We had some trouble loading more responses. Please try again.') gettext('Additional responses could not be loaded. Refresh the page to try again.')
); );
} }
} }
......
common/static/common/js/discussion/views/response_comment_view.js
...@@ -114,8 +114,8 @@ ...@@ -114,8 +114,8 @@
}, },
error: function() { error: function() {
return DiscussionUtil.discussionAlert( return DiscussionUtil.discussionAlert(
gettext('Sorry'), gettext('Error'),
gettext('We had some trouble deleting this comment. Please try again.') gettext('This comment could not be deleted. Refresh the page to try again.')
); );
} }
}); });
......
common/static/common/js/spec/discussion/utils_spec.js
...@@ -29,7 +29,7 @@ ...@@ -29,7 +29,7 @@
}); });
spyOn(DiscussionUtil, 'discussionAlert'); spyOn(DiscussionUtil, 'discussionAlert');
DiscussionUtil.safeAjax.calls.mostRecent().args[0].error(); DiscussionUtil.safeAjax.calls.mostRecent().args[0].error();
expect(DiscussionUtil.discussionAlert).toHaveBeenCalledWith('Sorry', 'error message'); expect(DiscussionUtil.discussionAlert).toHaveBeenCalledWith('Error', 'error message');
deferred.reject(); deferred.reject();
return expect(model.attributes).toEqual({ return expect(model.attributes).toEqual({
hello: false, hello: false,
......
common/static/common/js/spec/discussion/view/discussion_thread_list_view_spec.js
...@@ -550,7 +550,7 @@ ...@@ -550,7 +550,7 @@
it('does not add a search alert when no alternate term was searched', function() { it('does not add a search alert when no alternate term was searched', function() {
testCorrection(this.view, null); testCorrection(this.view, null);
expect(this.view.addSearchAlert.calls.count()).toEqual(1); expect(this.view.addSearchAlert.calls.count()).toEqual(1);
return expect(this.view.addSearchAlert.calls.mostRecent().args[0]).toMatch(/no threads matched/i); return expect(this.view.addSearchAlert.calls.mostRecent().args[0]).toMatch(/no posts matched/i);
}); });
it('clears search alerts when a new search is performed', function() { it('clears search alerts when a new search is performed', function() {
......
common/test/acceptance/tests/discussion/helpers.py
...@@ -12,6 +12,7 @@ from common.test.acceptance.fixtures.discussion import ( ...@@ -12,6 +12,7 @@ from common.test.acceptance.fixtures.discussion import (
Thread, Thread,
Response, Response,
ForumsConfigMixin, ForumsConfigMixin,
MultipleThreadFixture,
) )
from common.test.acceptance.pages.lms.discussion import DiscussionTabSingleThreadPage from common.test.acceptance.pages.lms.discussion import DiscussionTabSingleThreadPage
from common.test.acceptance.tests.helpers import UniqueCourseTest from common.test.acceptance.tests.helpers import UniqueCourseTest
......
common/test/acceptance/tests/discussion/test_discussion.py
...@@ -33,7 +33,8 @@ from common.test.acceptance.fixtures.discussion import ( ...@@ -33,7 +33,8 @@ from common.test.acceptance.fixtures.discussion import (
Response, Response,
Comment, Comment,
SearchResult, SearchResult,
MultipleThreadFixture) MultipleThreadFixture,
)
from common.test.acceptance.tests.discussion.helpers import BaseDiscussionMixin from common.test.acceptance.tests.discussion.helpers import BaseDiscussionMixin
from common.test.acceptance.tests.helpers import skip_if_browser from common.test.acceptance.tests.helpers import skip_if_browser
...@@ -416,7 +417,7 @@ class DiscussionTabSingleThreadTest(BaseDiscussionTestCase, DiscussionResponsePa ...@@ -416,7 +417,7 @@ class DiscussionTabSingleThreadTest(BaseDiscussionTestCase, DiscussionResponsePa
self.assertFalse(self.thread_page.is_comment_deletable("comment1")) self.assertFalse(self.thread_page.is_comment_deletable("comment1"))
class DiscussionTabMultipleThreadTest(BaseDiscussionTestCase): class DiscussionTabMultipleThreadTest(BaseDiscussionTestCase, BaseDiscussionMixin):
""" """
Tests for the discussion page with multiple threads Tests for the discussion page with multiple threads
""" """
...@@ -441,19 +442,6 @@ class DiscussionTabMultipleThreadTest(BaseDiscussionTestCase): ...@@ -441,19 +442,6 @@ class DiscussionTabMultipleThreadTest(BaseDiscussionTestCase):
) )
self.thread_page_1.visit() self.thread_page_1.visit()
@attr(shard=2)
def setup_multiple_threads(self, thread_count):
threads = []
for i in range(thread_count):
thread_id = "test_thread_{}_{}".format(i, uuid4().hex)
thread_body = "Dummy Long text body." * 50
threads.append(
Thread(id=thread_id, commentable_id=self.discussion_id, body=thread_body),
)
self.thread_ids.append(thread_id)
view = MultipleThreadFixture(threads)
view.push()
@attr('a11y') @attr('a11y')
def test_page_accessibility(self): def test_page_accessibility(self):
self.thread_page_1.a11y_audit.config.set_rules({ self.thread_page_1.a11y_audit.config.set_rules({
...@@ -1039,7 +1027,9 @@ class InlineDiscussionTest(UniqueCourseTest, DiscussionResponsePaginationTestMix ...@@ -1039,7 +1027,9 @@ class InlineDiscussionTest(UniqueCourseTest, DiscussionResponsePaginationTestMix
""" """
Tests Inline Discussion for accessibility issues. Tests Inline Discussion for accessibility issues.
""" """
self.setup_multiple_inline_threads(thread_count=3) self.setup_multiple_threads(thread_count=3)
# First test the a11y of the expanded list of threads
self.discussion_page.expand_discussion() self.discussion_page.expand_discussion()
self.discussion_page.a11y_audit.config.set_rules({ self.discussion_page.a11y_audit.config.set_rules({
'ignore': [ 'ignore': [
...@@ -1048,6 +1038,14 @@ class InlineDiscussionTest(UniqueCourseTest, DiscussionResponsePaginationTestMix ...@@ -1048,6 +1038,14 @@ class InlineDiscussionTest(UniqueCourseTest, DiscussionResponsePaginationTestMix
}) })
self.discussion_page.a11y_audit.check_for_accessibility_errors() self.discussion_page.a11y_audit.check_for_accessibility_errors()
# Now show the first thread and test the a11y again
self.discussion_page.show_thread(self.thread_ids[0])
self.discussion_page.a11y_audit.check_for_accessibility_errors()
# Finally show the new post form and test its a11y
self.discussion_page.click_new_post_button()
self.discussion_page.a11y_audit.check_for_accessibility_errors()
def test_add_a_post_is_present_if_can_create_thread_when_expanded(self): def test_add_a_post_is_present_if_can_create_thread_when_expanded(self):
self.discussion_page.expand_discussion() self.discussion_page.expand_discussion()
# Add a Post link is present # Add a Post link is present
...@@ -1363,7 +1361,7 @@ class DiscussionSearchAlertTest(UniqueCourseTest): ...@@ -1363,7 +1361,7 @@ class DiscussionSearchAlertTest(UniqueCourseTest):
def test_no_rewrite(self): def test_no_rewrite(self):
self.setup_corrected_text(None) self.setup_corrected_text(None)
self.page.perform_search() self.page.perform_search()
self.check_search_alert_messages(["no threads"]) self.check_search_alert_messages(["no posts"])
@attr(shard=2) @attr(shard=2)
def test_rewrite_dismiss(self): def test_rewrite_dismiss(self):
...@@ -1385,7 +1383,7 @@ class DiscussionSearchAlertTest(UniqueCourseTest): ...@@ -1385,7 +1383,7 @@ class DiscussionSearchAlertTest(UniqueCourseTest):
self.setup_corrected_text(None) self.setup_corrected_text(None)
self.page.perform_search() self.page.perform_search()
self.check_search_alert_messages(["no threads"]) self.check_search_alert_messages(["no posts"])
@attr(shard=2) @attr(shard=2)
def test_rewrite_and_user(self): def test_rewrite_and_user(self):
...@@ -1397,7 +1395,7 @@ class DiscussionSearchAlertTest(UniqueCourseTest): ...@@ -1397,7 +1395,7 @@ class DiscussionSearchAlertTest(UniqueCourseTest):
def test_user_only(self): def test_user_only(self):
self.setup_corrected_text(None) self.setup_corrected_text(None)
self.page.perform_search(self.SEARCHED_USERNAME) self.page.perform_search(self.SEARCHED_USERNAME)
self.check_search_alert_messages(["no threads", self.SEARCHED_USERNAME]) self.check_search_alert_messages(["no posts", self.SEARCHED_USERNAME])
# make sure clicking the link leads to the user profile page # make sure clicking the link leads to the user profile page
UserProfileViewFixture([]).push() UserProfileViewFixture([]).push()
self.page.get_search_alert_links().first.click() self.page.get_search_alert_links().first.click()
......
lms/djangoapps/teams/static/teams/js/views/team_discussion.js
...@@ -6,14 +6,16 @@ ...@@ -6,14 +6,16 @@
define(['backbone', 'underscore', 'gettext', 'common/js/discussion/views/discussion_inline_view'], define(['backbone', 'underscore', 'gettext', 'common/js/discussion/views/discussion_inline_view'],
function(Backbone, _, gettext, DiscussionInlineView) { function(Backbone, _, gettext, DiscussionInlineView) {
var TeamDiscussionView = Backbone.View.extend({ var TeamDiscussionView = Backbone.View.extend({
initialize: function() { initialize: function(options) {
window.$$course_id = this.$el.data('course-id'); window.$$course_id = this.$el.data('course-id');
this.readOnly = options.readOnly;
}, },
render: function() { render: function() {
var discussionInlineView = new DiscussionInlineView({ var discussionInlineView = new DiscussionInlineView({
el: this.$el, el: this.$el,
showByDefault: true showByDefault: true,
readOnly: this.readOnly
}); });
discussionInlineView.render(); discussionInlineView.render();
return this; return this;
......
lms/djangoapps/teams/static/teams/js/views/team_profile.js
...@@ -56,7 +56,7 @@ ...@@ -56,7 +56,7 @@
); );
this.discussionView = new TeamDiscussionView({ this.discussionView = new TeamDiscussionView({
el: this.$('.discussion-module'), el: this.$('.discussion-module'),
readOnly: this.$('.discussion-module').data('read-only') === true readOnly: !isMember
}); });
this.discussionView.render(); this.discussionView.render();
......
lms/static/sass/discussion/_discussion-v1.scss
...@@ -54,7 +54,6 @@ ...@@ -54,7 +54,6 @@
.discussion-module { .discussion-module {
.discussion { .discussion {
clear: both; clear: both;
padding-top: ($baseline/2);
} }
.btn { .btn {
......
lms/static/sass/discussion/_discussion.scss
...@@ -296,9 +296,6 @@ body.discussion { ...@@ -296,9 +296,6 @@ body.discussion {
@extend .discussion-body; @extend .discussion-body;
display: block; display: block;
position: relative; position: relative;
margin: $baseline 0;
padding: $baseline;
border: 1px solid $forum-color-border !important;
border-radius: $forum-border-radius; border-radius: $forum-border-radius;
header { header {
...@@ -319,6 +316,7 @@ body.discussion { ...@@ -319,6 +316,7 @@ body.discussion {
.discussion-module-header { .discussion-module-header {
@include float(left); @include float(left);
width: flex-grid(7); width: flex-grid(7);
margin-bottom: ($baseline * 0.75);
} }
.add_post_btn_container { .add_post_btn_container {
...@@ -399,6 +397,9 @@ section.discussion { ...@@ -399,6 +397,9 @@ section.discussion {
.xblock-student_view-discussion { .xblock-student_view-discussion {
@extend %ui-print-excluded; @extend %ui-print-excluded;
// Overrides overspecific courseware CSS from:
// https://github.com/edx/edx-platform/blob/master/lms/static/sass/course/courseware/_courseware.scss#L499
padding-top: 15px !important;
} }
// ==================== // ====================
......
lms/static/sass/discussion/elements/_navigation.scss
...@@ -126,7 +126,6 @@ ...@@ -126,7 +126,6 @@
.forum-nav-refine-bar { .forum-nav-refine-bar {
@include clearfix(); @include clearfix();
@include border-radius($forum-border-radius, $forum-border-radius, 0, 0); @include border-radius($forum-border-radius, $forum-border-radius, 0, 0);
@include text-align(right);
font-size: $forum-small-font-size; font-size: $forum-small-font-size;
border-bottom: 1px solid $forum-color-border; border-bottom: 1px solid $forum-color-border;
background-color: $gray-l5; background-color: $gray-l5;
...@@ -135,16 +134,18 @@ ...@@ -135,16 +134,18 @@
} }
.forum-nav-filter-main { .forum-nav-filter-main {
@include text-align(left);
@include float(left);
box-sizing: border-box; box-sizing: border-box;
display: inline-block; display: inline-block;
width: 50%; width: 50%;
@include text-align(left);
} }
.forum-nav-filter-cohort, .forum-nav-sort { .forum-nav-filter-cohort, .forum-nav-sort {
@include text-align(right);
@include float(right);
box-sizing: border-box; box-sizing: border-box;
display: inline-block; display: inline-block;
@include text-align(right);
@media (min-width: $bp-screen-md) { @media (min-width: $bp-screen-md) {
width: 50%; width: 50%;
...@@ -174,7 +175,7 @@ ...@@ -174,7 +175,7 @@
// Thread list // Thread list
// ----------- // -----------
.forum-nav-thread-list { .forum-nav-thread-list {
@include padding-left(0); padding-left: 0 !important; // should *not* be RTLed, see below for explanation
margin: 0; margin: 0;
overflow-y: scroll; overflow-y: scroll;
list-style: none; list-style: none;
...@@ -182,6 +183,10 @@ ...@@ -182,6 +183,10 @@
.forum-nav-thread-labels { .forum-nav-thread-labels {
margin: 5px 0 0; margin: 5px 0 0;
// Overrides overspecific courseware CSS from:
// https://github.com/edx/edx-platform/blob/master/lms/static/sass/course/courseware/_courseware.scss#L470
// note this should *not* be RTLed, as the rule it overrides is not RTLed
padding-left: 0 !important;
} }
.thread-preview-body { .thread-preview-body {
...@@ -216,6 +221,9 @@ ...@@ -216,6 +221,9 @@
.forum-nav-thread-link { .forum-nav-thread-link {
@include border-left(3px solid transparent); @include border-left(3px solid transparent);
@include rtl {
flex-direction: row-reverse;
}
display: flex; display: flex;
padding: $baseline / 2; padding: $baseline / 2;
transition: none; transition: none;
...@@ -290,6 +298,7 @@ ...@@ -290,6 +298,7 @@
.forum-nav-thread-wrapper-0 { .forum-nav-thread-wrapper-0 {
@extend %forum-nav-thread-wrapper; @extend %forum-nav-thread-wrapper;
@include margin-right($baseline/5); @include margin-right($baseline/5);
align-self: flex-start;
.icon { .icon {
font-size: $forum-base-font-size; font-size: $forum-base-font-size;
......
lms/static/sass/discussion/utilities/_variables-v1.scss
...@@ -24,7 +24,7 @@ $forum-color-read-post: $forum-color-primary !default; ...@@ -24,7 +24,7 @@ $forum-color-read-post: $forum-color-primary !default;
$forum-color-never-read-post: $gray-d3 !default; $forum-color-never-read-post: $gray-d3 !default;
$forum-color-editor-preview-label: $gray-d2 !default; $forum-color-editor-preview-label: $gray-d2 !default;
$forum-color-response-count: $gray-d2 !default; $forum-color-response-count: $gray-d2 !default;
$forum-color-navigation-bar: $forum-color-primary !default; $forum-color-navigation-bar: #f6f6f6 !default;
// post images // post images
$post-image-dimension: ($baseline*3) !default; // image size + margin $post-image-dimension: ($baseline*3) !default; // image size + margin
......
lms/static/sass/discussion/utilities/_variables-v2.scss
...@@ -24,7 +24,7 @@ $forum-color-read-post: palette(grayscale, base) !default; ...@@ -24,7 +24,7 @@ $forum-color-read-post: palette(grayscale, base) !default;
$forum-color-never-read-post: $forum-color-primary !default; $forum-color-never-read-post: $forum-color-primary !default;
$forum-color-editor-preview-label: palette(grayscale, base) !default; $forum-color-editor-preview-label: palette(grayscale, base) !default;
$forum-color-response-count: palette(grayscale, base) !default; $forum-color-response-count: palette(grayscale, base) !default;
$forum-color-navigation-bar: $forum-color-primary !default; $forum-color-navigation-bar: palette(grayscale, x-back) !default;
// post images // post images
$post-image-dimension: ($baseline*3) !default; // image size + margin $post-image-dimension: ($baseline*3) !default; // image size + margin
......
lms/static/sass/discussion/views/_inline.scss
...@@ -2,8 +2,6 @@ ...@@ -2,8 +2,6 @@
// ==================== // ====================
.discussion.inline-discussion { .discussion.inline-discussion {
padding-top: $baseline;
.inline-threads { .inline-threads {
border: 1px solid $forum-color-border; border: 1px solid $forum-color-border;
border-radius: $forum-border-radius; border-radius: $forum-border-radius;
...@@ -14,10 +12,18 @@ ...@@ -14,10 +12,18 @@
border-radius: $forum-border-radius; border-radius: $forum-border-radius;
.forum-nav-bar { .forum-nav-bar {
background-color: $forum-color-navigation-bar; color: $forum-color-navigation-bar;
padding: ($baseline / 2) $baseline;
position: relative;
.all-posts-btn {
color: $forum-color-primary;
.btn-link { .icon {
color: $forum-color-active-text; position: absolute;
@include left(7px);
top: 17px;
}
} }
} }
......
lms/static/sass/discussion/views/_thread.scss
...@@ -27,8 +27,13 @@ ...@@ -27,8 +27,13 @@
} }
.post-body { .post-body {
@include float(left);
width: flex-grid(10,12); width: flex-grid(10,12);
} }
.post-context {
@include float(left);
}
} }
.posted-details { .posted-details {
......
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