-
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 4 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 |
---|---|---|
|
@@ -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.