From 5dad2afa44793ca06da21b97f05c074e936f2e58 Mon Sep 17 00:00:00 2001 From: Brian Jacobel Date: Thu, 10 Nov 2016 11:43:37 -0500 Subject: [PATCH] Squashed minor issue fixes in inline discussion view Make this selector more specific, was getting overrides Fix Teams implementation of inline discussion Fix LMS tests Work on pagination support Jasmine tests for this feature Focus works when navigating between list and detail Fix safe template violations --- .../views/discussion_inline_view.js | 44 +++-- .../view/discussion_inline_view_spec.js | 154 ++++++++++++++++++ .../js/spec_helpers/discussion_spec_helper.js | 38 +++-- .../discussion/inline-discussion.underscore | 5 +- .../test/acceptance/pages/lms/discussion.py | 2 +- .../tests/discussion/test_discussion.py | 14 ++ .../discussion/discussion_board.html | 2 +- .../teams/js/spec/views/team_profile_spec.js | 2 +- .../static/teams/js/views/team_discussion.js | 3 +- .../teams/templates/teams/teams.html | 4 + lms/static/sass/discussion/_discussion.scss | 6 +- .../sass/discussion/elements/_navigation.scss | 76 +++++---- lms/static/sass/discussion/views/_inline.scss | 18 +- 13 files changed, 295 insertions(+), 73 deletions(-) create mode 100644 common/static/common/js/spec/discussion/view/discussion_inline_view_spec.js diff --git a/common/static/common/js/discussion/views/discussion_inline_view.js b/common/static/common/js/discussion/views/discussion_inline_view.js index 5332d75868..10e4ead4cd 100644 --- a/common/static/common/js/discussion/views/discussion_inline_view.js +++ b/common/static/common/js/discussion/views/discussion_inline_view.js @@ -19,11 +19,28 @@ } }, + page_re: /\?discussion_page=(\d+)/, + initialize: function(options) { + var match; + this.$el = options.el; + this.showByDefault = options.showByDefault || false; this.toggleDiscussionBtn = this.$('.discussion-show'); - this.newPostForm = this.$el.find('.new-post-article'); - this.listenTo(this.newPostView, 'newPost:cancel', this.hideNewPost); + this.listenTo(this.model, 'change', this.render); + + match = this.page_re.exec(window.location.href); + if (match) { + this.page = parseInt(match[1], 10); + } else { + this.page = 1; + } + + // By default the view is displayed in a hidden state. If you want it to be shown by default (e.g. in Teams) + // pass showByDefault as an option. This code will open it on initialization. + if (this.showByDefault) { + this.toggleDiscussion(); + } }, loadDiscussions: function($elem, error) { @@ -46,7 +63,7 @@ }, renderDiscussion: function($elem, response, textStatus, discussionId) { - var $discussion, + var discussionHtml, user = new DiscussionUser(response.user_info), self = this; @@ -59,21 +76,21 @@ this.course_settings = new DiscussionCourseSettings(response.course_settings); - this.discussion = new Discussion(); + this.discussion = new Discussion(undefined, {pages: response.num_pages}); this.discussion.reset(response.discussion_data, { silent: false }); - $discussion = _.template($('#inline-discussion-template').html())({ + discussionHtml = edx.HtmlUtils.template($('#inline-discussion-template').html())({ threads: response.discussion_data, read_only: this.readOnly, discussionId: discussionId }); if (this.$('section.discussion').length) { - this.$('section.discussion').replaceWith($discussion); + edx.HtmlUtils.setHtml(this.$el, discussionHtml); } else { - this.$el.append($discussion); + edx.HtmlUtils.append(this.$el, discussionHtml); } this.threadListView = new DiscussionThreadListView({ @@ -138,6 +155,9 @@ // Show the thread list view this.threadListView.$el.removeClass('is-hidden'); + + // Set focus to thread list item that was saved as active + this.threadListView.$('.is-active').focus(); }, toggleDiscussion: function() { @@ -147,9 +167,10 @@ this.hideDiscussion(); } else { this.toggleDiscussionBtn.addClass('shown'); - this.toggleDiscussionBtn.find('.button-text').html(gettext('Hide Discussion')); + this.toggleDiscussionBtn.find('.button-text').text(gettext('Hide Discussion')); if (this.retrieved) { this.$('section.discussion').slideDown(); + this.$('section.discussion').removeClass('is-hidden'); this.showed = true; } else { this.loadDiscussions(this.$el, function() { @@ -165,8 +186,9 @@ hideDiscussion: function() { this.$('section.discussion').slideUp(); + this.$('section.discussion').addClass('is-hidden'); this.toggleDiscussionBtn.removeClass('shown'); - this.toggleDiscussionBtn.find('.button-text').html(gettext('Show Discussion')); + this.toggleDiscussionBtn.find('.button-text').text(gettext('Show Discussion')); this.showed = false; }, @@ -182,13 +204,15 @@ } else { this.newPostForm.show().focus(); } + this.newPostView.$el.removeClass('is-hidden'); this.toggleDiscussionBtn.addClass('shown'); - this.toggleDiscussionBtn.find('.button-text').html(gettext('Hide Discussion')); + this.toggleDiscussionBtn.find('.button-text').text(gettext('Hide Discussion')); this.$('section.discussion').slideDown(); this.showed = true; }, hideNewPost: function() { + this.newPostView.$el.addClass('is-hidden'); return this.newPostForm.slideUp(300); } }); diff --git a/common/static/common/js/spec/discussion/view/discussion_inline_view_spec.js b/common/static/common/js/spec/discussion/view/discussion_inline_view_spec.js new file mode 100644 index 0000000000..2f31058183 --- /dev/null +++ b/common/static/common/js/spec/discussion/view/discussion_inline_view_spec.js @@ -0,0 +1,154 @@ +/* globals + _, Discussion, DiscussionCourseSettings, DiscussionViewSpecHelper, DiscussionSpecHelper, + DiscussionInlineView, DiscussionUtil, DiscussionThreadShowView, Thread + */ +(function() { + 'use strict'; + describe('DiscussionInlineView', function() { + var createTestView, showDiscussion, setNextAjaxResult, + TEST_THREAD_TITLE = 'Test thread title'; + + beforeEach(function() { + DiscussionSpecHelper.setUpGlobals(); + setFixtures( + '
' + + '
' + + '

Test Discussion

' + + '
' + + ' Topic: Category / Target ' + + '
' + + '
' + + ' ' + + '
' + ); + DiscussionSpecHelper.setUnderscoreFixtures(); + this.ajaxSpy = spyOn($, 'ajax'); + + // Don't attempt to render markdown + spyOn(DiscussionUtil, 'makeWmdEditor'); + spyOn(DiscussionThreadShowView.prototype, 'convertMath'); + }); + + createTestView = function() { + var testView = new DiscussionInlineView({ + el: $('.discussion-module') + }); + testView.render(); + return testView; + }; + + showDiscussion = function(test, testView) { + setNextAjaxResult(test, { + user_info: DiscussionSpecHelper.getTestUserInfo(), + roles: DiscussionSpecHelper.getTestRoleInfo(), + course_settings: DiscussionSpecHelper.createTestCourseSettings().attributes, + discussion_data: DiscussionViewSpecHelper.makeThreadWithProps({ + commentable_id: 'test-topic', + title: TEST_THREAD_TITLE + }), + page: 1, + num_pages: 1 + }); + testView.$('.discussion-show').click(); + }; + + setNextAjaxResult = function(test, result) { + test.ajaxSpy.and.callFake(function(params) { + var deferred = $.Deferred(); + deferred.resolve(); + params.success(result); + return deferred; + }); + }; + + describe('inline discussion', function() { + it('is shown after "Show Discussion" is clicked', function() { + var testView = createTestView(this), + showButton = testView.$('.discussion-show'); + showDiscussion(this, testView); + + // Verify that the discussion is now shown + expect(showButton).toHaveClass('shown'); + expect(showButton.text().trim()).toEqual('Hide Discussion'); + expect(testView.$('.inline-discussion:visible')).not.toHaveClass('is-hidden'); + }); + + it('is hidden after "Hide Discussion" is clicked', function() { + var testView = createTestView(this), + showButton = testView.$('.discussion-show'); + showDiscussion(this, testView); + + // Hide the discussion by clicking the toggle button again + testView.$('.discussion-show').click(); + + // Verify that the discussion is now hidden + expect(showButton).not.toHaveClass('shown'); + expect(showButton.text().trim()).toEqual('Show Discussion'); + expect(testView.$('.inline-discussion:visible')).toHaveClass('is-hidden'); + }); + }); + + describe('new post form', function() { + it('should not be visible when the discussion is first shown', function() { + var testView = createTestView(this); + showDiscussion(this, testView); + expect(testView.$('.new-post-article')).toHaveClass('is-hidden'); + }); + + it('should be shown when the "Add a Post" button is clicked', function() { + var testView = createTestView(this); + showDiscussion(this, testView); + testView.$('.new-post-btn').click(); + expect(testView.$('.new-post-article')).not.toHaveClass('is-hidden'); + }); + + it('should be hidden when the "Cancel" button is clicked', function() { + var testView = createTestView(this); + showDiscussion(this, testView); + testView.$('.new-post-btn').click(); + testView.$('.forum-new-post-form .cancel').click(); + expect(testView.$('.new-post-article')).toHaveClass('is-hidden'); + }); + }); + + describe('thread listing', function() { + it('builds a view that lists the threads', function() { + var testView = createTestView(this); + showDiscussion(this, testView); + expect(testView.$('.forum-nav-thread-title').text()).toBe(TEST_THREAD_TITLE); + }); + }); + + describe('thread post drill down', function() { + it('can drill down to a thread', function() { + var testView = createTestView(this); + showDiscussion(this, testView); + testView.$('.forum-nav-thread-link').click(); + + // Verify that the list of threads is hidden + expect(testView.$('.inline-threads')).toHaveClass('is-hidden'); + + // Verify that the individual thread is shown + expect(testView.$('.group-visibility-label').text().trim()).toBe('This post is visible to everyone.'); + }); + + it('can go back to the list of threads', function() { + var testView = createTestView(this); + showDiscussion(this, testView); + testView.$('.forum-nav-thread-link').click(); + testView.$('.all-posts-btn').click(); + + // Verify that the list of threads is shown + expect(testView.$('.inline-threads')).not.toHaveClass('is-hidden'); + + // Verify that the individual thread is no longer shown + expect(testView.$('.group-visibility-label').length).toBe(0); + }); + }); + }); +}()); diff --git a/common/static/common/js/spec_helpers/discussion_spec_helper.js b/common/static/common/js/spec_helpers/discussion_spec_helper.js index 8e96ea5382..7a05b8d8ff 100644 --- a/common/static/common/js/spec_helpers/discussion_spec_helper.js +++ b/common/static/common/js/spec_helpers/discussion_spec_helper.js @@ -1,4 +1,4 @@ -/* global Content, Discussion, DiscussionCourseSettings, DiscussionUtil, DiscussionUser */ +/* global _, Content, Discussion, DiscussionCourseSettings, DiscussionUtil, DiscussionUser */ (function() { 'use strict'; this.DiscussionSpecHelper = (function() { @@ -51,23 +51,29 @@ return jasmine.createSpyObj('event', ['preventDefault', 'target']); }; - DiscussionSpecHelper.createTestCourseSettings = function() { - return new DiscussionCourseSettings({ - category_map: { - children: [['Test Topic', 'entry'], ['Other Topic', 'entry']], - entries: { - 'Test Topic': { - is_cohorted: true, - id: 'test_topic' - }, - 'Other Topic': { - is_cohorted: true, - id: 'other_topic' + DiscussionSpecHelper.createTestCourseSettings = function(options) { + var context = _.extend( + { + category_map: { + children: [['Test Topic', 'entry'], ['Other Topic', 'entry']], + entries: { + 'Test Topic': { + is_cohorted: true, + id: 'test_topic' + }, + 'Other Topic': { + is_cohorted: true, + id: 'other_topic' + } } - } + }, + is_cohorted: true, + allow_anonymous: false, + allow_anonymous_to_peers: false }, - is_cohorted: true - }); + options || {} + ); + return new DiscussionCourseSettings(context); }; DiscussionSpecHelper.createTestDiscussion = function(options) { diff --git a/common/static/common/templates/discussion/inline-discussion.underscore b/common/static/common/templates/discussion/inline-discussion.underscore index 97162b6451..b2081125f4 100644 --- a/common/static/common/templates/discussion/inline-discussion.underscore +++ b/common/static/common/templates/discussion/inline-discussion.underscore @@ -1,9 +1,9 @@ -
+
-
+
@@ -19,3 +19,4 @@
+ diff --git a/common/test/acceptance/pages/lms/discussion.py b/common/test/acceptance/pages/lms/discussion.py index 7ba2af8cf7..13223e6124 100644 --- a/common/test/acceptance/pages/lms/discussion.py +++ b/common/test/acceptance/pages/lms/discussion.py @@ -504,7 +504,7 @@ class InlineDiscussionPage(PageObject): ).fulfill() def get_num_displayed_threads(self): - return len(self._find_within(".discussion-thread")) + return len(self._find_within(".forum-nav-thread")) def has_thread(self, thread_id): """Returns true if this page is showing the thread with the specified id.""" diff --git a/common/test/acceptance/tests/discussion/test_discussion.py b/common/test/acceptance/tests/discussion/test_discussion.py index 80a0b0b188..9cba009e4c 100644 --- a/common/test/acceptance/tests/discussion/test_discussion.py +++ b/common/test/acceptance/tests/discussion/test_discussion.py @@ -1067,6 +1067,20 @@ class InlineDiscussionTest(UniqueCourseTest, DiscussionResponsePaginationTestMix # Check if 'thread-wrapper' is focused after expanding thread self.assertFalse(thread_page.check_if_selector_is_focused(selector='.thread-wrapper')) + @attr('a11y') + def test_inline_a11y(self): + """ + Tests Inline Discussion for accessibility issues. + """ + self.setup_multiple_inline_threads(thread_count=3) + self.discussion_page.expand_discussion() + self.discussion_page.a11y_audit.config.set_rules({ + 'ignore': [ + 'section' + ] + }) + self.discussion_page.a11y_audit.check_for_accessibility_errors() + def test_add_a_post_is_present_if_can_create_thread_when_expanded(self): self.discussion_page.expand_discussion() # Add a Post link is present diff --git a/lms/djangoapps/discussion/templates/discussion/discussion_board.html b/lms/djangoapps/discussion/templates/discussion/discussion_board.html index 7882739cf1..a6cc5cfe1e 100644 --- a/lms/djangoapps/discussion/templates/discussion/discussion_board.html +++ b/lms/djangoapps/discussion/templates/discussion/discussion_board.html @@ -82,7 +82,7 @@ DiscussionBoardFactory({
- +
diff --git a/lms/djangoapps/teams/static/teams/js/spec/views/team_profile_spec.js b/lms/djangoapps/teams/static/teams/js/spec/views/team_profile_spec.js index 2021c3d36e..83fa3768f7 100644 --- a/lms/djangoapps/teams/static/teams/js/spec/views/team_profile_spec.js +++ b/lms/djangoapps/teams/static/teams/js/spec/views/team_profile_spec.js @@ -101,7 +101,7 @@ define([ it('can render itself', function() { var requests = AjaxHelpers.requests(this), view = createTeamProfileView(requests, {}); - expect(view.$('.discussion-thread').length).toEqual(3); + expect(view.$('.forum-nav-thread').length).toEqual(3); }); it('shows New Post button when user joins a team', function() { diff --git a/lms/djangoapps/teams/static/teams/js/views/team_discussion.js b/lms/djangoapps/teams/static/teams/js/views/team_discussion.js index dd3dac878b..9661127ac7 100644 --- a/lms/djangoapps/teams/static/teams/js/views/team_discussion.js +++ b/lms/djangoapps/teams/static/teams/js/views/team_discussion.js @@ -12,7 +12,8 @@ render: function() { var discussionInlineView = new DiscussionInlineView({ - el: this.$el + el: this.$el, + showByDefault: true }); discussionInlineView.render(); return this; diff --git a/lms/djangoapps/teams/templates/teams/teams.html b/lms/djangoapps/teams/templates/teams/teams.html index fa58f64b76..9482ea8b8d 100644 --- a/lms/djangoapps/teams/templates/teams/teams.html +++ b/lms/djangoapps/teams/templates/teams/teams.html @@ -1,4 +1,7 @@ ## mako + +<%page expression_filter="h"/> + <%! import json %> <%! from django.utils.translation import ugettext as _ @@ -55,3 +58,4 @@ from openedx.core.djangolib.js_utils import ( <%include file="../discussion/_underscore_templates.html" /> +<%include file="../discussion/_thread_list_template.html" /> diff --git a/lms/static/sass/discussion/_discussion.scss b/lms/static/sass/discussion/_discussion.scss index 13a120096b..f98180d172 100644 --- a/lms/static/sass/discussion/_discussion.scss +++ b/lms/static/sass/discussion/_discussion.scss @@ -323,8 +323,8 @@ body.discussion { .add_post_btn_container { @include text-align(right); - position: relative; - top: -25px; + width: flex-grid(12); + height: (2 * $baseline); } div.add-response.post-extended-content { @@ -356,8 +356,6 @@ section.discussion { } .new-post-article { - display: none; - .inner-wrapper { max-width: 1180px; min-width: 760px; diff --git a/lms/static/sass/discussion/elements/_navigation.scss b/lms/static/sass/discussion/elements/_navigation.scss index 2fcf7331e4..75e05c7e6a 100644 --- a/lms/static/sass/discussion/elements/_navigation.scss +++ b/lms/static/sass/discussion/elements/_navigation.scss @@ -194,10 +194,14 @@ } } -// Overrides underspecific style from courseware css: -// https://github.com/edx/edx-platform/blob/master/lms/static/sass/course/courseware/_courseware.scss#L402 -.course-wrapper .course-content .forum-nav-thread-list li { - margin: 0; +// Overrides underspecific styles from courseware css +.course-wrapper .course-content .forum-nav-thread-list-wrapper .forum-nav-thread-list { + @include padding-left(0); + list-style: none; + + li { + margin: 0; + } } .forum-nav-thread { @@ -229,36 +233,6 @@ background-color: $forum-color-hover-thread; } - &.is-active { - color: $forum-color-background; - background-color: $forum-color-reading-thread; - - .forum-nav-thread-labels > li { - border-color: $forum-color-background; - color: $forum-color-background; - } - - .forum-nav-thread-votes-count { - color: $forum-color-background; - } - - .forum-nav-thread-comments-count { - color: $base-font-color; - - &:after { - @include border-right-color($forum-color-border); - } - } - - span.icon { - color: $forum-color-background; - } - - .thread-preview-body { - color: $forum-color-background; - } - } - .forum-nav-thread-unread-comments-count { display: inline-block; font-size: $forum-small-font-size; @@ -266,6 +240,8 @@ } } + .discussion:not(.inline-discussion) + &.never-read { .forum-nav-thread-link { @include border-left(3px solid $forum-color-never-read-post); @@ -274,6 +250,38 @@ } } +.discussion:not(.inline-discussion) .forum-nav-thread { + .forum-nav-thread-link.is-active { + color: $forum-color-background; + background-color: $forum-color-reading-thread; + + .forum-nav-thread-labels > li { + border-color: $forum-color-background; + color: $forum-color-background; + } + + .forum-nav-thread-votes-count { + color: $forum-color-background; + } + + .forum-nav-thread-comments-count { + color: $base-font-color; + + &:after { + @include border-right-color($forum-color-border); + } + } + + span.icon { + color: $forum-color-background; + } + + .thread-preview-body { + color: $forum-color-background; + } + } +} + %forum-nav-thread-wrapper { display: inline-block; vertical-align: middle; diff --git a/lms/static/sass/discussion/views/_inline.scss b/lms/static/sass/discussion/views/_inline.scss index 7218d7e8f5..8c8557a73e 100644 --- a/lms/static/sass/discussion/views/_inline.scss +++ b/lms/static/sass/discussion/views/_inline.scss @@ -1,8 +1,8 @@ // forums - inline discussion styling // ==================== -.inline-discussion { - padding-top: $baseline * 1.5; +.discussion.inline-discussion { + padding-top: $baseline; .inline-threads { border: 1px solid $forum-color-border; @@ -23,8 +23,20 @@ .forum-content { padding: $baseline / 2; - max-height: 500px; overflow-y: auto; } } + + .wmd-preview-container { + @include discussion-new-post-wmd-preview-container; + margin-bottom: $baseline; + } + + .wmd-preview-label { + @include discussion-wmd-preview-label; + } + + .wmd-preview { + @include discussion-wmd-preview; + } }