-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
pagination: Fix disappearing pagination on back nav and refresh #1060
Changes from all commits
1124bf2
6fd6247
a0e4327
3ea74a1
2d197de
9fb33f9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ import Navigation from './models/navigation'; | |
import AlternativeVerticals from './models/alternativeverticals'; | ||
import DirectAnswer from './models/directanswer'; | ||
import LocationBias from './models/locationbias'; | ||
import QueryTriggers from './models/querytriggers'; | ||
|
||
import StorageKeys from './storage/storagekeys'; | ||
import AnalyticsEvent from './analytics/analyticsevent'; | ||
|
@@ -172,7 +173,9 @@ export default class Core { | |
} | ||
|
||
const locationRadiusFilterNode = this.getLocationRadiusFilterNode(); | ||
|
||
const queryTrigger = this.getQueryTriggerForSearchApi( | ||
this.globalStorage.getState(StorageKeys.QUERY_TRIGGER) | ||
); | ||
return this._searcher | ||
.verticalSearch(verticalKey, { | ||
limit: this.globalStorage.getState(StorageKeys.SEARCH_CONFIG).limit, | ||
|
@@ -184,7 +187,7 @@ export default class Core { | |
offset: this.globalStorage.getState(StorageKeys.SEARCH_OFFSET) || 0, | ||
isDynamicFiltersEnabled: this._isDynamicFiltersEnabled, | ||
skipSpellCheck: this.globalStorage.getState('skipSpellCheck'), | ||
queryTrigger: this.globalStorage.getState('queryTrigger'), | ||
queryTrigger: queryTrigger, | ||
sessionTrackingEnabled: this.globalStorage.getState(StorageKeys.SESSIONS_OPT_IN), | ||
sortBys: this.globalStorage.getState(StorageKeys.SORT_BYS), | ||
locationRadius: locationRadiusFilterNode ? locationRadiusFilterNode.getFilter().value : null, | ||
|
@@ -217,7 +220,7 @@ export default class Core { | |
this.globalStorage.set(StorageKeys.LOCATION_BIAS, data[StorageKeys.LOCATION_BIAS]); | ||
} | ||
this.globalStorage.delete('skipSpellCheck'); | ||
this.globalStorage.delete('queryTrigger'); | ||
this.globalStorage.delete(StorageKeys.QUERY_TRIGGER); | ||
|
||
const exposedParams = { | ||
verticalKey: verticalKey, | ||
|
@@ -280,11 +283,14 @@ export default class Core { | |
this.globalStorage.set(StorageKeys.SPELL_CHECK, {}); | ||
this.globalStorage.set(StorageKeys.LOCATION_BIAS, {}); | ||
|
||
const queryTrigger = this.getQueryTriggerForSearchApi( | ||
this.globalStorage.getState(StorageKeys.QUERY_TRIGGER) | ||
); | ||
return this._searcher | ||
.universalSearch(queryString, { | ||
geolocation: this.globalStorage.getState(StorageKeys.GEOLOCATION), | ||
skipSpellCheck: this.globalStorage.getState('skipSpellCheck'), | ||
queryTrigger: this.globalStorage.getState('queryTrigger'), | ||
queryTrigger: queryTrigger, | ||
sessionTrackingEnabled: this.globalStorage.getState(StorageKeys.SESSIONS_OPT_IN), | ||
context: context, | ||
referrerPageUrl: referrerPageUrl | ||
|
@@ -299,7 +305,7 @@ export default class Core { | |
this.globalStorage.set(StorageKeys.SPELL_CHECK, data[StorageKeys.SPELL_CHECK]); | ||
this.globalStorage.set(StorageKeys.LOCATION_BIAS, data[StorageKeys.LOCATION_BIAS]); | ||
this.globalStorage.delete('skipSpellCheck'); | ||
this.globalStorage.delete('queryTrigger'); | ||
this.globalStorage.delete(StorageKeys.QUERY_TRIGGER); | ||
|
||
const exposedParams = { | ||
queryString: queryString, | ||
|
@@ -497,6 +503,18 @@ export default class Core { | |
this.filterRegistry.clearLocationRadiusFilterNode(); | ||
} | ||
|
||
/** | ||
* Returns the query trigger for the search API given the SDK query trigger | ||
* @param {QueryTriggers} queryTrigger SDK query trigger | ||
* @returns {QueryTriggers} query trigger if accepted by the search API, null o/w | ||
*/ | ||
getQueryTriggerForSearchApi (queryTrigger) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we want to use queryTrigger both for persistent storage magic and also as something completely separate we send to the api, I think it would be easier to just have them be two separate things. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Talked offline. This is definitely not an ideal way to get context about entry for the query update. This is necessary because we have /two/ ways we need to fix (on a refresh and on a popstate from back nav). Both of these initiate a search through a query update. Discussed w @tmeyer2115 a bit yesterday about the second point. QueryTriggers will be a first class SDK enum. We will guide behavior based on this storage key and if it has a certain value, we will also pass it in the LiveAPI request. Keeping this implementation for now, I understand the refactoring worries:( |
||
if (queryTrigger === QueryTriggers.QUERY_PARAMETER) { | ||
return null; | ||
} | ||
return queryTrigger; | ||
} | ||
|
||
enableDynamicFilters () { | ||
this._isDynamicFiltersEnabled = true; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
/** @module QueryTriggers */ | ||
|
||
/** | ||
* QueryTriggers is an ENUM of the possible triggers for a | ||
* query update. | ||
* | ||
* @enum {string} | ||
*/ | ||
export default { | ||
INITIALIZE: 'initialize', | ||
QUERY_PARAMETER: 'query-parameter' | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ | |
import Component from '../component'; | ||
import DOM from '../../dom/dom'; | ||
import StorageKeys from '../../../core/storage/storagekeys'; | ||
import QueryTriggers from '../../../core/models/querytriggers'; | ||
import SearchParams from '../../dom/searchparams'; | ||
|
||
const IconState = { | ||
|
@@ -137,6 +138,15 @@ export default class SearchComponent extends Component { | |
*/ | ||
this._defaultInitialSearch = this._globalSearchConfig.defaultInitialSearch; | ||
|
||
/** | ||
* The default options for core search | ||
* @type {Object} | ||
*/ | ||
this._defaultSearchOptions = { | ||
setQueryParams: true, | ||
resetPagination: !!this._verticalKey | ||
}; | ||
|
||
/** | ||
* The query string to use for the input box, provided to template for rendering. | ||
* Optionally provided | ||
|
@@ -150,11 +160,22 @@ export default class SearchComponent extends Component { | |
} | ||
if (q === null) { | ||
if (this._defaultInitialSearch || this._defaultInitialSearch === '') { | ||
this.core.globalStorage.set(StorageKeys.QUERY_TRIGGER, QueryTriggers.INITIALIZE); | ||
this.core.setQuery(this._defaultInitialSearch); | ||
} | ||
return; | ||
} | ||
this.debouncedSearch(q); | ||
|
||
const queryTrigger = this.core.globalStorage.getState(StorageKeys.QUERY_TRIGGER); | ||
const resetPagination = this._verticalKey && | ||
queryTrigger !== QueryTriggers.QUERY_PARAMETER && | ||
queryTrigger !== QueryTriggers.INITIALIZE; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we anticipate making a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was not sure what to anticipate! We currently have another query trigger (for SpellCheck component, which is more dynamic, relying on the |
||
const searchOptions = Object.assign( | ||
{}, | ||
this._defaultSearchOptions, | ||
{ resetPagination: resetPagination } | ||
); | ||
this.debouncedSearch(q, searchOptions); | ||
}); | ||
|
||
/** | ||
|
@@ -462,7 +483,7 @@ export default class SearchComponent extends Component { | |
this.core.persistentStorage.delete(StorageKeys.SEARCH_OFFSET); | ||
this.core.globalStorage.delete(StorageKeys.SEARCH_OFFSET); | ||
this.core.setQuery(query); | ||
this.debouncedSearch(query); | ||
this.debouncedSearch(query, this._defaultSearchOptions); | ||
return false; | ||
} | ||
|
||
|
@@ -505,9 +526,10 @@ export default class SearchComponent extends Component { | |
* performed if we recently searched, if there's no query for universal search, or if this | ||
* is a twin searchbar. | ||
* @param {string} query The string to query against. | ||
* @param {Object} searchOptions The options to pass for core search | ||
* @returns {Promise} A promise that will perform the query and update globalStorage accordingly. | ||
*/ | ||
debouncedSearch (query) { | ||
debouncedSearch (query, searchOptions) { | ||
if (this._throttled || | ||
(!query && !this._verticalKey) || | ||
(!query && this._verticalKey && !this._allowEmptySearch) || | ||
|
@@ -535,10 +557,10 @@ export default class SearchComponent extends Component { | |
lng: position.coords.longitude, | ||
radius: position.coords.accuracy | ||
}); | ||
resolve(this.search(query)); | ||
resolve(this.search(query, searchOptions)); | ||
}, | ||
() => { | ||
resolve(this.search(query)); | ||
resolve(this.search(query, searchOptions)); | ||
const { enabled, message } = this._geolocationTimeoutAlert; | ||
if (enabled) { | ||
window.alert(message); | ||
|
@@ -547,29 +569,23 @@ export default class SearchComponent extends Component { | |
this._geolocationOptions) | ||
); | ||
} else { | ||
return this.search(query); | ||
return this.search(query, searchOptions); | ||
} | ||
}); | ||
} else { | ||
return this.search(query); | ||
return this.search(query, searchOptions); | ||
} | ||
} | ||
|
||
/** | ||
* Performs a query using the provided string input. | ||
* @param {string} query The string to query against. | ||
* @param {Object} searchOptions The options to pass for core search | ||
* @returns {Promise} A promise that will perform the query and update globalStorage accordingly. | ||
*/ | ||
search (query) { | ||
search (query, searchOptions) { | ||
if (this._verticalKey) { | ||
this.core.verticalSearch( | ||
this._config.verticalKey, | ||
{ | ||
resetPagination: true, | ||
setQueryParams: true | ||
}, | ||
{ input: query } | ||
); | ||
this.core.verticalSearch(this._config.verticalKey, searchOptions, { input: query }); | ||
} else { | ||
// NOTE(billy) Temporary hack for DEMO | ||
// Remove me after the demo | ||
|
@@ -592,10 +608,10 @@ export default class SearchComponent extends Component { | |
urls[tabs[i].configId] = url; | ||
} | ||
} | ||
return this.core.search(query, urls, { setQueryParams: true }); | ||
return this.core.search(query, urls, searchOptions); | ||
} | ||
|
||
return this.core.search(query, undefined, { setQueryParams: true }); | ||
return this.core.search(query, undefined, searchOptions); | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,7 +31,7 @@ export default class SpellCheckComponent extends Component { | |
|
||
onCreate () { | ||
this.core.persistentStorage.delete('skipSpellCheck', true); | ||
this.core.persistentStorage.delete('queryTrigger', true); | ||
this.core.persistentStorage.delete(StorageKeys.QUERY_TRIGGER, true); | ||
} | ||
|
||
setState (data, val) { | ||
|
@@ -49,7 +49,7 @@ export default class SpellCheckComponent extends Component { | |
let params = new SearchParams(window.location.search.substring(1)); | ||
params.set('query', query.value); | ||
params.set('skipSpellCheck', true); | ||
Comment on lines
50
to
51
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Maybe we should use StorageKeys here as well? Everything else LGTM! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree! But I think that would be a change that should not be in this commit (as that is outside of the scope of this work). I can make an item for the more sweeping change of using StorageKeys wherever possible with global/persistent storage. SLAP-621 |
||
params.set('queryTrigger', type.toLowerCase()); | ||
params.set(StorageKeys.QUERY_TRIGGER, type.toLowerCase()); | ||
return '?' + params.toString(); | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See note in the commit message. I made SLAP-616 to address going from presence to absence of query param on back nav.