From b2134aa1731fd0346969969cac55b56a7a603253 Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Tue, 9 Aug 2016 13:22:14 -0400 Subject: [PATCH] Refactor full page search for style, remove lookups --- .../controllers/full-page-search.js.es6 | 36 ++++++--- .../initializers/page-tracking.js.es6 | 17 ----- .../discourse/lib/page-tracker.js.es6 | 21 +++++- .../discourse/routes/full-page-search.js.es6 | 8 +- .../discourse/templates/full-page-search.hbs | 74 +++++++++---------- .../acceptance/search-full-test.js.es6 | 6 +- 6 files changed, 85 insertions(+), 77 deletions(-) diff --git a/app/assets/javascripts/discourse/controllers/full-page-search.js.es6 b/app/assets/javascripts/discourse/controllers/full-page-search.js.es6 index 27ee3f030..b2a5aeaec 100644 --- a/app/assets/javascripts/discourse/controllers/full-page-search.js.es6 +++ b/app/assets/javascripts/discourse/controllers/full-page-search.js.es6 @@ -4,6 +4,7 @@ import showModal from 'discourse/lib/show-modal'; import { default as computed, observes } from 'ember-addons/ember-computed-decorators'; import Category from 'discourse/models/category'; import { escapeExpression } from 'discourse/lib/utilities'; +import { setTransient } from 'discourse/lib/page-tracker'; const SortOrders = [ {name: I18n.t('search.relevance'), id: 0}, @@ -14,6 +15,7 @@ const SortOrders = [ export default Ember.Controller.extend({ needs: ["application"], + bulkSelectEnabled: null, loading: Em.computed.not("model"), queryParams: ["q", "context_id", "context", "skip_context"], @@ -26,10 +28,15 @@ export default Ember.Controller.extend({ sortOrders: SortOrders, @computed('model.posts') - resultCount(posts){ + resultCount(posts) { return posts && posts.length; }, + @computed('resultCount') + hasResults(resultCount) { + return (resultCount || 0) > 0; + }, + @computed('q') hasAutofocus(q) { return Em.isEmpty(q); @@ -85,7 +92,7 @@ export default Ember.Controller.extend({ setSearchTerm(term) { this._searchOnSortChange = false; if (term) { - SortOrders.forEach((order) => { + SortOrders.forEach(order => { if (term.indexOf(order.term) > -1){ this.set('sortOrder', order.id); term = term.replace(order.term, ""); @@ -98,7 +105,7 @@ export default Ember.Controller.extend({ }, @observes('sortOrder') - triggerSearch(){ + triggerSearch() { if (this._searchOnSortChange) { this.search(); } @@ -130,13 +137,22 @@ export default Ember.Controller.extend({ this.set("controllers.application.showFooter", !this.get("loading")); }, - canBulkSelect: Em.computed.alias('currentUser.staff'), + @computed('hasResults') + canBulkSelect(hasResults) { + return this.currentUser && this.currentUser.staff && hasResults; + }, + + @computed + canCreateTopic() { + return this.currentUser && !this.site.mobileView; + }, search(){ if (this.get("searching")) return; this.set("searching", true); - const router = Discourse.__container__.lookup('router:main'); + this.set('bulkSelectEnabled', false); + this.get('selected').clear(); var args = { q: this.get("searchTerm") }; @@ -160,9 +176,9 @@ export default Ember.Controller.extend({ ajax("/search", { data: args }).then(results => { const model = translateResults(results) || {}; - router.transientCache('lastSearch', { searchKey, model }, 5); + setTransient('lastSearch', { searchKey, model }, 5); this.set("model", model); - }).finally(() => { this.set("searching",false); }); + }).finally(() => this.set("searching", false)); }, actions: { @@ -188,16 +204,12 @@ export default Ember.Controller.extend({ }, refresh() { - this.set('bulkSelectEnabled', false); - this.get('selected').clear(); this.search(); }, showSearchHelp() { // TODO: dupe code should be centralized - ajax("/static/search_help.html", { dataType: 'html' }).then((model) => { - showModal('searchHelp', { model }); - }); + ajax("/static/search_help.html", { dataType: 'html' }).then(model => showModal('searchHelp', { model })); }, search() { diff --git a/app/assets/javascripts/discourse/initializers/page-tracking.js.es6 b/app/assets/javascripts/discourse/initializers/page-tracking.js.es6 index 860ddaf0f..7e744a7de 100644 --- a/app/assets/javascripts/discourse/initializers/page-tracking.js.es6 +++ b/app/assets/javascripts/discourse/initializers/page-tracking.js.es6 @@ -7,31 +7,14 @@ export default { initialize(container) { - const cache = {}; - var transitionCount = 0; - // Tell our AJAX system to track a page transition const router = container.lookup('router:main'); router.on('willTransition', viewTrackingRequired); router.on('didTransition', function() { Em.run.scheduleOnce('afterRender', Ember.Route, cleanDOM); - transitionCount++; - _.each(cache, (v,k) => { - if (v && v.target && v.target < transitionCount) { - delete cache[k]; - } - }); }); - router.transientCache = function(key, data, count) { - if (data === undefined) { - return cache[key]; - } else { - return cache[key] = {data, target: transitionCount + count}; - } - }; - startPageTracking(router); // Out of the box, Discourse tries to track google analytics diff --git a/app/assets/javascripts/discourse/lib/page-tracker.js.es6 b/app/assets/javascripts/discourse/lib/page-tracker.js.es6 index 7e63b5c73..89deaafe0 100644 --- a/app/assets/javascripts/discourse/lib/page-tracker.js.es6 +++ b/app/assets/javascripts/discourse/lib/page-tracker.js.es6 @@ -2,6 +2,18 @@ const PageTracker = Ember.Object.extend(Ember.Evented); let _pageTracker = PageTracker.create(); let _started = false; + +const cache = {}; +let transitionCount = 0; + +export function setTransient(key, data, count) { + cache[key] = {data, target: transitionCount + count}; +} + +export function getTransient(key) { + return cache[key]; +} + export function startPageTracking(router) { if (_started) { return; } @@ -11,8 +23,13 @@ export function startPageTracking(router) { // Refreshing the title is debounced, so we need to trigger this in the // next runloop to have the correct title. - Em.run.next(() => { - _pageTracker.trigger('change', url, Discourse.get('_docTitle')); + Em.run.next(() => _pageTracker.trigger('change', url, Discourse.get('_docTitle'))); + + transitionCount++; + _.each(cache, (v,k) => { + if (v && v.target && v.target < transitionCount) { + delete cache[k]; + } }); }); _started = true; diff --git a/app/assets/javascripts/discourse/routes/full-page-search.js.es6 b/app/assets/javascripts/discourse/routes/full-page-search.js.es6 index 8507c0dce..10cd87655 100644 --- a/app/assets/javascripts/discourse/routes/full-page-search.js.es6 +++ b/app/assets/javascripts/discourse/routes/full-page-search.js.es6 @@ -2,13 +2,13 @@ import { ajax } from 'discourse/lib/ajax'; import { translateResults, getSearchKey, isValidSearchTerm } from "discourse/lib/search"; import Composer from 'discourse/models/composer'; import PreloadStore from 'preload-store'; +import { getTransient, setTransient } from 'discourse/lib/page-tracker'; export default Discourse.Route.extend({ queryParams: { q: {}, context_id: {}, context: {}, skip_context: {} }, model(params) { - const router = Discourse.__container__.lookup('router:main'); - var cached = router.transientCache('lastSearch'); + const cached = getTransient('lastSearch'); var args = { q: params.q }; if (params.context_id && !args.skip_context) { args.search_context = { @@ -21,7 +21,7 @@ export default Discourse.Route.extend({ if (cached && cached.data.searchKey === searchKey) { // extend expiry - router.transientCache('lastSearch', { searchKey, model: cached.data.model }, 5); + setTransient('lastSearch', { searchKey, model: cached.data.model }, 5); return cached.data.model; } @@ -33,7 +33,7 @@ export default Discourse.Route.extend({ } }).then(results => { const model = (results && translateResults(results)) || {}; - router.transientCache('lastSearch', { searchKey, model }, 5); + setTransient('lastSearch', { searchKey, model }, 5); return model; }); }, diff --git a/app/assets/javascripts/discourse/templates/full-page-search.hbs b/app/assets/javascripts/discourse/templates/full-page-search.hbs index b869f0a72..d44095631 100644 --- a/app/assets/javascripts/discourse/templates/full-page-search.hbs +++ b/app/assets/javascripts/discourse/templates/full-page-search.hbs @@ -1,61 +1,57 @@
{{search-text-field value=searchTerm class="full-page-search input-xxlarge search no-blur" action="search" hasAutofocus=hasAutofocus}} {{d-button action="search" icon="search" class="btn-primary" disabled=searchButtonDisabled}} - {{#if currentUser}} - {{#unless site.mobileView}} - {{d-button id="create-topic" class="btn-default" action="createTopic" actionParam=searchTerm icon="plus" label="topic.create"}} - {{/unless}} + + {{#if canCreateTopic}} + {{d-button id="create-topic" class="btn-default" action="createTopic" actionParam=searchTerm icon="plus" label="topic.create"}} {{/if}} + {{#if canBulkSelect}} - {{#if model.posts}} - {{d-button icon="list" class="bulk-select" title="topics.bulk.toggle" action="toggleBulkSelect"}} - {{bulk-select-button selected=selected action="refresh"}} - {{/if}} + {{d-button icon="list" class="bulk-select" title="topics.bulk.toggle" action="toggleBulkSelect"}} + {{bulk-select-button selected=selected action="refresh"}} {{/if}}
-{{#if model.posts}} - {{#if bulkSelectEnabled}} +{{#if bulkSelectEnabled}}
- {{i18n "search.select_all"}} - {{i18n "search.clear_all"}} + {{d-link action="selectAll" label="search.select_all"}} + {{d-link action="clearAll" label="search.clear_all"}}
- {{/if}} {{/if}} {{#if context}} -
- -
+
+ +
{{/if}} {{#conditional-loading-spinner condition=loading}} - {{#unless model.posts}} + {{#unless hasResults}}

{{#if searchActive}} - {{i18n "search.no_results"}} + {{i18n "search.no_results"}} {{/if}} {{i18n "search.search_help"}}

{{/unless}} - {{#if model.posts}} -
-
- - {{{i18n "search.result_count" count=resultCount term=noSortQ}}} - + {{#if hasResults}} +
+
+ + {{{i18n "search.result_count" count=resultCount term=noSortQ}}} + +
+
+ + {{i18n "search.sort_by"}} + + {{combo-box value=sortOrder content=sortOrders castInteger="true"}} +
-
- - {{i18n "search.sort_by"}} - - {{combo-box value=sortOrder content=sortOrders castInteger="true"}} -
-
{{/if}} {{#each model.posts as |result|}} @@ -101,7 +97,7 @@ {{#if showLikeCount}} {{#if result.like_count}} {{/if}} {{/if}} @@ -109,11 +105,11 @@
{{/each}} - {{#if model.posts}} -

- {{i18n "search.no_more_results"}} - {{i18n "search.search_help"}} -

+ {{#if hasResults}} +

+ {{i18n "search.no_more_results"}} + {{i18n "search.search_help"}} +

{{/if}} {{/conditional-loading-spinner}} diff --git a/test/javascripts/acceptance/search-full-test.js.es6 b/test/javascripts/acceptance/search-full-test.js.es6 index 005ea036f..889b066ee 100644 --- a/test/javascripts/acceptance/search-full-test.js.es6 +++ b/test/javascripts/acceptance/search-full-test.js.es6 @@ -6,16 +6,16 @@ test("perform various searches", assert => { andThen(() => { assert.ok(find('input.search').length > 0); - assert.ok(find('.topic').length === 0); + assert.ok(find('.fps-topic').length === 0); }); fillIn('.search input', 'none'); click('.search .btn-primary'); - andThen(() => assert.ok(find('.topic').length === 0), 'has no results'); + andThen(() => assert.ok(find('.fps-topic').length === 0), 'has no results'); fillIn('.search input', 'posts'); click('.search .btn-primary'); - andThen(() => assert.ok(find('.topic').length === 1, 'has one post')); + andThen(() => assert.ok(find('.fps-topic').length === 1, 'has one post')); });