From 1f2c843f23475cb18efde656d0efc36b2360486b Mon Sep 17 00:00:00 2001 From: Brian Jacobel Date: Tue, 2 Aug 2016 16:07:13 -0400 Subject: [PATCH] Move search box to header. Add search results to breadcrumbs when you search --- .../views/discussion_thread_list_view.js | 32 +++--------- .../view/discussion_thread_list_view_spec.js | 33 ------------ .../test/acceptance/pages/lms/discussion.py | 2 +- .../tests/discussion/test_discussion.py | 8 +++ .../discussion/js/discussion_board_factory.js | 35 +++++++++++-- .../spec/views/discussion_search_view_spec.js | 36 +++++++++++++ .../js/views/discussion_search_view.js | 50 ++++++++++++++++++ .../discussion/templates/search.underscore | 9 ++++ .../discussion/discussion_board.html | 4 ++ lms/static/lms/js/spec/main.js | 1 + lms/static/sass/discussion/_build.scss | 1 + .../sass/discussion/elements/_navigation.scss | 51 +++++++++---------- .../sass/discussion/utilities/_shame.scss | 22 -------- lms/static/sass/discussion/views/_search.scss | 8 +++ .../discussion/_thread_list_template.html | 9 ---- .../ux/reference/course-skeleton.html | 19 ++++--- 16 files changed, 190 insertions(+), 130 deletions(-) create mode 100644 lms/djangoapps/discussion/static/discussion/js/spec/views/discussion_search_view_spec.js create mode 100644 lms/djangoapps/discussion/static/discussion/js/views/discussion_search_view.js create mode 100644 lms/djangoapps/discussion/static/discussion/templates/search.underscore create mode 100644 lms/static/sass/discussion/views/_search.scss diff --git a/common/static/common/js/discussion/views/discussion_thread_list_view.js b/common/static/common/js/discussion/views/discussion_thread_list_view.js index c2764e86e1..6375c7aeb0 100644 --- a/common/static/common/js/discussion/views/discussion_thread_list_view.js +++ b/common/static/common/js/discussion/views/discussion_thread_list_view.js @@ -98,8 +98,6 @@ 'keyup .forum-nav-browse-filter-input': 'filterTopics', 'click .forum-nav-browse-menu-wrapper': 'ignoreClick', 'click .forum-nav-browse-title': 'selectTopicHandler', - 'keydown .forum-nav-search-input': 'performSearch', - 'click .fa-search': 'performSearch', 'change .forum-nav-sort-control': 'sortThreads', 'click .forum-nav-thread-link': 'threadSelected', 'click .forum-nav-load-more-link': 'loadMorePages', @@ -591,7 +589,6 @@ DiscussionThreadListView.prototype.selectTopic = function($target) { var allItems, discussionIds, $item; this.hideBrowseMenu(); - this.clearSearch(); $item = $target.closest('.forum-nav-browse-menu-item'); this.setCurrentTopicDisplay(this.getPathText($item)); @@ -666,22 +663,15 @@ return this.retrieveFirstPage(event); }; - DiscussionThreadListView.prototype.performSearch = function(event) { - /* - event.which 13 represent the Enter button - */ - - var text; - if (event.which === 13 || event.type === 'click') { - event.preventDefault(); - this.hideBrowseMenu(); - this.setCurrentTopicDisplay(gettext('Search Results')); - text = this.$('.forum-nav-search-input').val(); - this.searchFor(text); - } + DiscussionThreadListView.prototype.performSearch = function($searchInput) { + this.hideBrowseMenu(); + this.setCurrentTopicDisplay(gettext('Search Results')); + // trigger this event so the breadcrumbs can update as well + this.trigger('search:initiated'); + this.searchFor($searchInput.val(), $searchInput); }; - DiscussionThreadListView.prototype.searchFor = function(text) { + DiscussionThreadListView.prototype.searchFor = function(text, $searchInput) { var url, self = this; this.clearSearchAlerts(); this.clearFilters(); @@ -696,7 +686,7 @@ */ return DiscussionUtil.safeAjax({ - $elem: this.$('.forum-nav-search-input'), + $elem: $searchInput, data: { text: text }, @@ -790,12 +780,6 @@ }); }; - DiscussionThreadListView.prototype.clearSearch = function() { - this.$('.forum-nav-search-input').val(''); - this.current_search = ''; - return this.clearSearchAlerts(); - }; - DiscussionThreadListView.prototype.clearFilters = function() { this.$('.forum-nav-filter-main-control').val('all'); return this.$('.forum-nav-filter-cohort-control').val('all'); diff --git a/common/static/common/js/spec/discussion/view/discussion_thread_list_view_spec.js b/common/static/common/js/spec/discussion/view/discussion_thread_list_view_spec.js index d55a902d80..6deea3d84f 100644 --- a/common/static/common/js/spec/discussion/view/discussion_thread_list_view_spec.js +++ b/common/static/common/js/spec/discussion/view/discussion_thread_list_view_spec.js @@ -374,24 +374,6 @@ }); }); - describe('Search events', function() { - it('perform search when enter pressed inside search textfield', function() { - setupAjax(); - spyOn(this.view, 'searchFor'); - this.view.$el.find('.forum-nav-search-input').trigger($.Event('keydown', { - which: 13 - })); - return expect(this.view.searchFor).toHaveBeenCalled(); - }); - - it('perform search when search icon is clicked', function() { - setupAjax(); - spyOn(this.view, 'searchFor'); - this.view.$el.find('.fa-search').click(); - return expect(this.view.searchFor).toHaveBeenCalled(); - }); - }); - describe('username search', function() { var setAjaxResults; @@ -582,14 +564,6 @@ return expectBrowseMenuVisible(false); }); - it('should hide when a search is executed', function() { - setupAjax(); - $('.forum-nav-search-input').trigger($.Event('keydown', { - which: 13 - })); - return expectBrowseMenuVisible(false); - }); - it('should hide when a category is clicked', function() { $('.forum-nav-browse-title')[0].click(); return expectBrowseMenuVisible(false); @@ -636,13 +610,6 @@ describe('selecting an item', function() { var testSelectionRequest; - it('should clear the search box', function() { - setupAjax(); - $('.forum-nav-search-input').val('foobar'); - $('.forum-nav-browse-menu-following .forum-nav-browse-title').click(); - return expect($('.forum-nav-search-input').val()).toEqual(''); - }); - it('should change the button text', function() { setupAjax(); $('.forum-nav-browse-menu-following .forum-nav-browse-title').click(); diff --git a/common/test/acceptance/pages/lms/discussion.py b/common/test/acceptance/pages/lms/discussion.py index 39abb9984c..3b5fbd9b18 100644 --- a/common/test/acceptance/pages/lms/discussion.py +++ b/common/test/acceptance/pages/lms/discussion.py @@ -678,7 +678,7 @@ class DiscussionTabHomePage(CoursePage, DiscussionPageMixin): return self.q(css=".discussion-body section.home-header").present def perform_search(self, text="dummy"): - self.q(css=".forum-nav-search-input").fill(text + chr(10)) + self.q(css=".search-input").fill(text + chr(10)) EmptyPromise( self.is_ajax_finished, "waiting for server to return result" diff --git a/common/test/acceptance/tests/discussion/test_discussion.py b/common/test/acceptance/tests/discussion/test_discussion.py index 14742745a6..78e17c2503 100644 --- a/common/test/acceptance/tests/discussion/test_discussion.py +++ b/common/test/acceptance/tests/discussion/test_discussion.py @@ -267,6 +267,14 @@ class DiscussionNavigationTest(BaseDiscussionTestCase): self.thread_page.q(css=".breadcrumbs .nav-item")[0].click() self.assertEqual(len(self.thread_page.q(css=".breadcrumbs .nav-item")), 1) + def test_breadcrumbs_clear_search(self): + self.thread_page.q(css=".search-input").fill("search text") + self.thread_page.q(css=".search-btn").click() + + # Verify that clicking the first breadcrumb clears your search + self.thread_page.q(css=".breadcrumbs .nav-item")[0].click() + self.assertEqual(self.thread_page.q(css=".search-input").text[0], "") + @attr(shard=2) class DiscussionTabSingleThreadTest(BaseDiscussionTestCase, DiscussionResponsePaginationTestMixin): diff --git a/lms/djangoapps/discussion/static/discussion/js/discussion_board_factory.js b/lms/djangoapps/discussion/static/discussion/js/discussion_board_factory.js index 103bd130e4..9cdc4ea7aa 100644 --- a/lms/djangoapps/discussion/static/discussion/js/discussion_board_factory.js +++ b/lms/djangoapps/discussion/static/discussion/js/discussion_board_factory.js @@ -7,9 +7,10 @@ 'backbone', 'discussion/js/discussion_router', 'discussion/js/views/discussion_fake_breadcrumbs', + 'discussion/js/views/discussion_search_view', 'common/js/discussion/views/new_post_view' ], - function($, Backbone, DiscussionRouter, DiscussionFakeBreadcrumbs, NewPostView) { + function($, Backbone, DiscussionRouter, DiscussionFakeBreadcrumbs, DiscussionSearchView, NewPostView) { return function(options) { var userInfo = options.user_info, sortPreference = options.sort_preference, @@ -22,7 +23,9 @@ newPostView, router, breadcrumbs, - BreadcrumbsModel; + BreadcrumbsModel, + searchBox, + routerEvents; // TODO: Perhaps eliminate usage of global variables when possible window.DiscussionUtil.loadRoles(options.roles); @@ -53,6 +56,13 @@ }); router.start(); + // Initialize and render search box + searchBox = new DiscussionSearchView({ + el: $('.forum-search'), + threadListView: router.nav + }).render(); + + // Initialize and render breadcrumbs BreadcrumbsModel = Backbone.Model.extend({ defaults: { contents: [] @@ -65,6 +75,7 @@ events: { 'click .all-topics': function(event) { event.preventDefault(); + searchBox.clearSearch(); this.model.set('contents', []); router.navigate('', {trigger: true}); router.nav.selectTopic($('.forum-nav-browse-menu-all')); @@ -72,9 +83,23 @@ } }).render(); - // Add new breadcrumbs when the user selects topics - router.nav.on('topic:selected', function(topic) { - breadcrumbs.model.set('contents', topic); + routerEvents = { + // Add new breadcrumbs and clear search box when the user selects topics + 'topic:selected': function(topic) { + breadcrumbs.model.set('contents', topic); + }, + // Clear search box when a thread is selected + 'thread:selected': function() { + searchBox.clearSearch(); + }, + // Add 'Search Results' to breadcrumbs when user searches + 'search:initiated': function() { + breadcrumbs.model.set('contents', ['Search Results']); + } + }; + + Object.keys(routerEvents).forEach(function(key) { + router.nav.on(key, routerEvents[key]); }); }; }); diff --git a/lms/djangoapps/discussion/static/discussion/js/spec/views/discussion_search_view_spec.js b/lms/djangoapps/discussion/static/discussion/js/spec/views/discussion_search_view_spec.js new file mode 100644 index 0000000000..f7c05f4daf --- /dev/null +++ b/lms/djangoapps/discussion/static/discussion/js/spec/views/discussion_search_view_spec.js @@ -0,0 +1,36 @@ +define([ + 'jquery', + 'edx-ui-toolkit/js/utils/constants', + 'discussion/js/views/discussion_search_view' +], + function($, constants, DiscussionSearchView) { + 'use strict'; + + describe('DiscussionSearchView', function() { + var view; + beforeEach(function() { + setFixtures('
'); + view = new DiscussionSearchView({ + el: $('.search-container'), + threadListView: { + performSearch: jasmine.createSpy() + } + }).render(); + }); + + describe('Search events', function() { + it('perform search when enter pressed inside search textfield', function() { + view.$el.find('.search-input').trigger($.Event('keydown', { + which: constants.keyCodes.enter + })); + expect(view.threadListView.performSearch).toHaveBeenCalled(); + }); + + it('perform search when search icon is clicked', function() { + view.$el.find('.search-btn').click(); + expect(view.threadListView.performSearch).toHaveBeenCalled(); + }); + }); + }); + } +); diff --git a/lms/djangoapps/discussion/static/discussion/js/views/discussion_search_view.js b/lms/djangoapps/discussion/static/discussion/js/views/discussion_search_view.js new file mode 100644 index 0000000000..9d1dd00620 --- /dev/null +++ b/lms/djangoapps/discussion/static/discussion/js/views/discussion_search_view.js @@ -0,0 +1,50 @@ +(function(define) { + 'use strict'; + + define([ + 'underscore', + 'backbone', + 'edx-ui-toolkit/js/utils/html-utils', + 'edx-ui-toolkit/js/utils/constants', + 'text!discussion/templates/search.underscore' + ], + function(_, Backbone, HtmlUtils, constants, searchTemplate) { + /* + * TODO: Much of the actual search functionality still takes place in discussion_thread_list_view.js + * Because of how it's structured there, extracting it is a massive task. Significant refactoring is needed + * in order to clean up that file and make it possible to break its logic into files like this one. + */ + var searchView = Backbone.View.extend({ + events: { + 'keydown .search-input': 'performSearch', + 'click .search-btn': 'performSearch', + 'topic:selected': 'clearSearch' + }, + initialize: function(options) { + _.extend(this, _.pick(options, 'threadListView')); + + this.template = HtmlUtils.template(searchTemplate); + this.threadListView = options.threadListView; + + this.listenTo(this.model, 'change', this.render); + this.render(); + }, + render: function() { + HtmlUtils.setHtml(this.$el, this.template()); + return this; + }, + performSearch: function(event) { + if (event.which === constants.keyCodes.enter || event.type === 'click') { + event.preventDefault(); + this.threadListView.performSearch($('.search-input', this.$el)); + } + }, + clearSearch: function() { + this.$('.search-input').val(''); + this.threadListView.clearSearchAlerts(); + } + }); + + return searchView; + }); +}).call(this, define || RequireJS.define); diff --git a/lms/djangoapps/discussion/static/discussion/templates/search.underscore b/lms/djangoapps/discussion/static/discussion/templates/search.underscore new file mode 100644 index 0000000000..3f289ce2a5 --- /dev/null +++ b/lms/djangoapps/discussion/static/discussion/templates/search.underscore @@ -0,0 +1,9 @@ + +" +/> + diff --git a/lms/djangoapps/discussion/templates/discussion/discussion_board.html b/lms/djangoapps/discussion/templates/discussion/discussion_board.html index b77b843ed8..98e7aae04f 100644 --- a/lms/djangoapps/discussion/templates/discussion/discussion_board.html +++ b/lms/djangoapps/discussion/templates/discussion/discussion_board.html @@ -55,16 +55,20 @@ DiscussionBoardFactory({ data-flag-moderator="${flag_moderator}" data-user-cohort-id="${user_cohort}">
+ ## Breadcrumb navigation
+ ## Add Post button % if has_permission(user, 'create_thread', course.id):
% endif + ## Search box +
diff --git a/lms/static/lms/js/spec/main.js b/lms/static/lms/js/spec/main.js index 72ed3adb50..a1ffbcd859 100644 --- a/lms/static/lms/js/spec/main.js +++ b/lms/static/lms/js/spec/main.js @@ -664,6 +664,7 @@ var testFiles = [ 'discussion/js/spec/discussion_board_factory_spec.js', 'discussion/js/spec/discussion_profile_page_factory_spec.js', + 'discussion/js/spec/views/discussion_search_view_spec.js', 'discussion/js/spec/views/discussion_user_profile_view_spec.js', 'lms/js/spec/preview/preview_factory_spec.js', 'js/spec/api_admin/catalog_preview_spec.js', diff --git a/lms/static/sass/discussion/_build.scss b/lms/static/sass/discussion/_build.scss index 67375adf8d..a5ed2f4d2c 100644 --- a/lms/static/sass/discussion/_build.scss +++ b/lms/static/sass/discussion/_build.scss @@ -36,5 +36,6 @@ $static-path: '../..' !default; @import 'views/thread'; @import 'views/create-edit-post'; @import 'views/response'; +@import 'views/search'; @import 'utilities/developer'; @import 'utilities/shame'; diff --git a/lms/static/sass/discussion/elements/_navigation.scss b/lms/static/sass/discussion/elements/_navigation.scss index cca01a90df..6c77bfd390 100644 --- a/lms/static/sass/discussion/elements/_navigation.scss +++ b/lms/static/sass/discussion/elements/_navigation.scss @@ -7,7 +7,30 @@ } // ------ -// Header +// Discussion Forums Page Header +// ------ +.discussion-board > .page-header { + $searchBoxPadding: rem($baseline / 2 + 2); + $searchBoxHeight: (rem($baseline) + ($searchBoxPadding * 2)); + + div { + display: inline-block; + vertical-align: middle; + } + + .page-header-main { + line-height: $searchBoxHeight; + } + + .page-header-secondary > .form-actions > button { + // Overrides base size set in lms/static/sass/shared-v2/_layouts.scss + // Done to match size of UXPL's search box. This is bad, I know. + height: $searchBoxHeight !important; + } +} + +// ------ +// Topic Listing Header // ------ .forum-nav-header { box-sizing: border-box; @@ -20,10 +43,8 @@ .forum-nav-browse { box-sizing: border-box; - // TODO: don't use table-cell for layout purposes as it switches the screenreader mode - display: table-cell; + width: 100%; vertical-align: middle; - width: auto; padding: 11px; border: 0; border-radius: 0; @@ -54,28 +75,6 @@ @include margin-left($baseline/4); } -.forum-nav-search { - box-sizing: border-box; - // TODO: don't use table-cell for layout purposes as it switches the screenreader mode - display: table-cell; - position: relative; - vertical-align: middle; - width: 50%; - padding: ($baseline/4); -} - -.forum-nav-search .icon { - font-size: $forum-small-font-size; - position: absolute; - margin-top: -6px; - top: 50%; - @include right($baseline/4 + 1px + $baseline / 4); // Wrapper padding + border + input padding -} - -.forum-nav-search-input { - width: 100%; -} - // ----------- // Browse menu // ----------- diff --git a/lms/static/sass/discussion/utilities/_shame.scss b/lms/static/sass/discussion/utilities/_shame.scss index 0784fb37fa..66ac9571a8 100644 --- a/lms/static/sass/discussion/utilities/_shame.scss +++ b/lms/static/sass/discussion/utilities/_shame.scss @@ -7,28 +7,6 @@ color: $black !important; } -// Override global label rules -.forum-nav-search label { - margin-bottom: 0; -} - -// Override global input rules -.forum-nav-search-input { - @include padding-left($baseline/4 !important); - @include padding-right($baseline/2 + 12px !important); // Leave room for icon - box-shadow: none !important; - border: 1px solid $forum-color-border !important; - border-radius: $forum-border-radius !important; - height: auto !important; - font-size: $forum-small-font-size !important; -} - -// Firefox does not compute the correct containing box for absolute positioning -// of .forum-nav-search .icon, so there's an extra div to make it happy -.forum-nav-search-ff-position-fix { - position: relative; -} - // Temporary breadcrumbs .has-breadcrumbs { .breadcrumbs { diff --git a/lms/static/sass/discussion/views/_search.scss b/lms/static/sass/discussion/views/_search.scss new file mode 100644 index 0000000000..bc607c0393 --- /dev/null +++ b/lms/static/sass/discussion/views/_search.scss @@ -0,0 +1,8 @@ +.forum-search { + display: inline-block; + margin-left: $baseline; + + .search-input { + width: input-width(short); + } +} diff --git a/lms/templates/discussion/_thread_list_template.html b/lms/templates/discussion/_thread_list_template.html index f5f78a1c3a..0094e70546 100644 --- a/lms/templates/discussion/_thread_list_template.html +++ b/lms/templates/discussion/_thread_list_template.html @@ -11,15 +11,6 @@ ${_("All Discussions")} -
-
- -
-
<%include file="_filter_dropdown.html" />
diff --git a/lms/templates/ux/reference/course-skeleton.html b/lms/templates/ux/reference/course-skeleton.html index c793092d24..f545d258a9 100644 --- a/lms/templates/ux/reference/course-skeleton.html +++ b/lms/templates/ux/reference/course-skeleton.html @@ -48,16 +48,15 @@ ## - update the Pattern Library's markup to match