pagination: Fix disappearing pagination on back nav and refresh #1060

creotutar · 2020-09-03T00:34:15Z

Previously, there were two pagination persistence bugs.

Land on a vertical page with a query with results. Go to page 2. Go
to page 3. Try to navigate backwards in browser history. You will go
to page 1 instead of page 3.
Land on a vertical page w a query w results. Go to page 2. Refresh
the page. You will be on page 1 instead of page 2.

Both of these related bugs were because we were defaulting to resetting
pagination on any searchbar search. In 1, backwards navigation updates the
query and the searchbar takes the updated query and searches. In 2,
landing with a query query parameter is determined in the SearchBar
component onCreate.

To fix this, we add another value for the pre-existing queryTrigger
global storage key, QUERY_PARAMETER, meaning the query was triggered by
a query parameter.

Note: as a result of this fix, we were no longer able to go from Page 2
to Page 1. This is because of another bug, where navigating backwards in
history from presence of a query param to absence of the param will not
erase the parameter in storage. This is fixed for the pagination param here
but should be addressed more widely in SLAP-616.

J=SLAP-610
TEST=manual

Test on a local Jambo HH Theme site attached to a local dist of the SDK.
Test on a local site using the SDK directly.

Test that navigating on a vertical to Page 2 and then Page 3 allows you
to navigate back in browser history to Page 2 and Page 1 respectively.

Test that navigating to a page and then refreshing maintains pagination
offset. (IE Test that query params for search-offset work as intended).

Test that navigating to a page and then searching another query in the
searchbar /will/ reset pagination (and remove the associated query
parameter).

Test that queryTrigger in the liveapi request in all the situations
are null.

Test that with defaultInitialSearch set, queryTrigger still sends as
'initialize' in the liveapi request.

The event constructor does not work in ie11, but we have an ie11 compliant implementation with DOM.trigger() T=https://yextops.zendesk.com/agent/tickets/348126 TEST=manual Test that before, in browserstack ie11, clicking the X button in a searchable facet has console error With code change, no console error and that it still clears the facet search input on chrome + ie11

Previously, there were two pagination persistence bugs. 1. Land on a vertical page with a query with results. Go to page 2. Go to page 3. Try to navigate backwards in browser history. You will go to page 1 instead of page 3. 2. Land on a vertical page w a query w results. Go to page 2. Refresh the page. You will be on page 1 instead of page 2. Both of these related bugs were because we were defaulting to resetting pagination on any searchbar search. In 1, backwards navigation updates the query and the searchbar takes the updated query and searches. In 2, landing with a `query` query parameter is determined in the SearchBar component onCreate. To fix this, we add another value for the pre-existing `queryTrigger` global storage key, QUERY_PARAMETER, meaning the query was triggered by a query parameter. Note: as a result of this fix, we were no longer able to go from Page 2 to Page 1. This is because of another bug, where navigating backwards in history from presence of a query param to absence of the param will not erase the parameter in storage. J=SLAP-610 TEST=manual Test on a local Jambo HH Theme site attached to a local dist of the SDK. Test on a local site using the SDK directly. Test that navigating on a vertical to Page 2 and then Page 3 allows you to navigate back in browser history to Page 2 and Page 1 respectively. Test that navigating to a page and then refreshing maintains pagination offset. (IE Test that query params for search-offset work as intended). Test that navigating to a page and then searching another query in the searchbar /will/ reset pagination (and remove the associated query parameter). Test that queryTrigger in the liveapi request in all the situations are null. Test that with defaultInitialSearch set, queryTrigger still sends as 'initialize' in the liveapi request.

src/ui/components/search/searchcomponent.js

creotutar · 2020-09-03T01:49:55Z

src/answers-umd.js

+          this.core.globalStorage.set(StorageKeys.QUERY_TRIGGER, QueryTriggers.QUERY_PARAMETER);
+        }
+
+        if (!data[StorageKeys.SEARCH_OFFSET]) {


See note in the commit message. I made SLAP-616 to address going from presence to absence of query param on back nav.

Listeners for 'update' don't work when you delete. This makes it so the number transformation doesn't happen for SEARCH_OFFSET.

…InitialSearch

cea2aj · 2020-09-03T22:37:16Z

src/ui/components/search/spellcheckcomponent.js

    params.set('query', query.value);
    params.set('skipSpellCheck', true);


nit: Maybe we should use StorageKeys here as well? Everything else LGTM!

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

alexisgrow

this LGTM!

tmeyer2115 · 2020-09-03T22:59:18Z

src/ui/components/search/searchcomponent.js

+      const queryTrigger = this.core.globalStorage.getState(StorageKeys.QUERY_TRIGGER);
+      const resetPagination = this._verticalKey &&
+        queryTrigger !== QueryTriggers.QUERY_PARAMETER &&
+        queryTrigger !== QueryTriggers.INITIALIZE;


Do we anticipate making a QueryTrigger for manual input in the search bar? If not, could we simplify this logic by saying if verticalKey and if !queryTrigger, then resetPagination?

I was not sure what to anticipate! We currently have another query trigger (for SpellCheck component, which is more dynamic, relying on the type of the data returned). I believe we would want spellcheck query triggers to continue to reset pagination. It felt easier here to enumerate these two than try to figure out the possible types for SpellCheck. How does that sound to you?

oshi97

Would it be possible to, instead of making these query trigger changes, update the resetListener (really just onpopstate) in answers-umd.js? I was thinking of maybe setting an IMPORTANT_SEARCH_OFFSET storage key onpopstate that would override the regular search_offset and would be deleted after every search. I was just thinking we want the fix this the easiest way that is to refactor, and if we can scope this change to just when the history changes I think that would be ideal?

oshi97 · 2020-09-03T23:14:24Z

src/core/core.js

+   * @param {QueryTriggers} queryTrigger SDK query trigger
+   * @returns {QueryTriggers} query trigger if accepted by the search API, null o/w
+   */
+  getQueryTriggerForSearchApi (queryTrigger) {


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.

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:(

Previously, there were two pagination persistence bugs. Land on a vertical page with a query with results. Go to page 2. Go to page 3. Try to navigate backwards in browser history. You will go to page 1 instead of page 3. Land on a vertical page w a query w results. Go to page 2. Refresh the page. You will be on page 1 instead of page 2. Both of these related bugs were because we were defaulting to resetting pagination on any searchbar search. In 1, backwards navigation updates the query and the searchbar takes the updated query and searches. In 2, landing with a query query parameter is determined in the SearchBar component onCreate. To fix this, we add another value for the pre-existing queryTrigger global storage key, QUERY_PARAMETER, meaning the query was triggered by a query parameter. Note: as a result of this fix, we were no longer able to go from Page 2 to Page 1. This is because of another bug, where navigating backwards in history from presence of a query param to absence of the param will not erase the parameter in storage. This is fixed for the pagination param here but should be addressed more widely in SLAP-616. J=SLAP-610 TEST=manual Test on a local Jambo HH Theme site attached to a local dist of the SDK. Test on a local site using the SDK directly. Test that navigating on a vertical to Page 2 and then Page 3 allows you to navigate back in browser history to Page 2 and Page 1 respectively. Test that navigating to a page and then refreshing maintains pagination offset. (IE Test that query params for search-offset work as intended). Test that navigating to a page and then searching another query in the searchbar /will/ reset pagination (and remove the associated query parameter). Test that queryTrigger in the liveapi request in all the situations are null. Test that with defaultInitialSearch set, queryTrigger still sends as 'initialize' in the liveapi request.

## Fixes: * Remove unsafe eval usages in a search bar only page (#1062) * Pagination: Fix disappearing pagination on back nav and refresh (#1060) * FilterOptions: fix ie11 issue with clearing searchable facets (#1058)

Add acceptance tests which cover uses of the browser back button and refresh page button These tests cover the persistence bugs that Christian addressed in #1060. My initial attempt was #1223, however we decided that several of those tests should be integration tests rather than acceptance tests. This PR includes only the tests from #1223 which make sense as acceptance tests. This PR includes utility functions for refreshing the page and using the back button. The navigation tab links were incorrectly pointing to './links' rather than './facets' for the Facets page, so I fixed that here as well. J=SLAP-868 TEST=auto

creotutar mentioned this pull request Sep 3, 2020

FilterOptions: replace new Event() with DOM.trigger() #1058

Merged

creotutar added 2 commits September 2, 2020 20:40

Fix pre-existing npm test, remove Event global

a0e4327

creotutar force-pushed the dev/pagination-back-nav branch from 9792642 to a0e4327 Compare September 3, 2020 00:41

creotutar commented Sep 3, 2020

View reviewed changes

src/ui/components/search/searchcomponent.js Outdated Show resolved Hide resolved

Use searchOptions not searchOptionsOverrides in fn signatures

3ea74a1

creotutar requested review from tmeyer2115, oshi97, alexisgrow and cea2aj and removed request for tmeyer2115 September 3, 2020 01:37

creotutar commented Sep 3, 2020

View reviewed changes

Set-0 instead of delete SEARCH_OFFSET

2d197de

Listeners for 'update' don't work when you delete. This makes it so the number transformation doesn't happen for SEARCH_OFFSET.

oshi97 force-pushed the hotfix/v1.5.5 branch from 1124bf2 to 8ef60ee Compare September 3, 2020 16:46

Add INITIALIZE as a non-reset pagination, set it when calling default…

9fb33f9

…InitialSearch

cea2aj reviewed Sep 3, 2020

View reviewed changes

alexisgrow approved these changes Sep 3, 2020

View reviewed changes

tmeyer2115 reviewed Sep 3, 2020

View reviewed changes

oshi97 reviewed Sep 3, 2020

View reviewed changes

tmeyer2115 self-requested a review September 4, 2020 00:02

tmeyer2115 approved these changes Sep 4, 2020

View reviewed changes

oshi97 approved these changes Sep 4, 2020

View reviewed changes

creotutar merged commit b32e087 into hotfix/v1.5.5 Sep 4, 2020

creotutar deleted the dev/pagination-back-nav branch September 15, 2020 14:12

oshi97 mentioned this pull request Oct 1, 2020

Version 1.5.5 #1117

Merged

This was referenced Jan 6, 2021

Storage Acceptance Tests #1223

Closed

Add persistence acceptance tests #1224

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pagination: Fix disappearing pagination on back nav and refresh #1060

pagination: Fix disappearing pagination on back nav and refresh #1060

creotutar commented Sep 3, 2020 •

edited

Loading

creotutar Sep 3, 2020

cea2aj Sep 3, 2020 •

edited

Loading

creotutar Sep 3, 2020

alexisgrow left a comment

tmeyer2115 Sep 3, 2020

creotutar Sep 3, 2020

oshi97 left a comment

oshi97 Sep 3, 2020

creotutar Sep 4, 2020

		params.set('query', query.value);
		params.set('skipSpellCheck', true);

pagination: Fix disappearing pagination on back nav and refresh #1060

pagination: Fix disappearing pagination on back nav and refresh #1060

Conversation

creotutar commented Sep 3, 2020 • edited Loading

creotutar Sep 3, 2020

Choose a reason for hiding this comment

cea2aj Sep 3, 2020 • edited Loading

Choose a reason for hiding this comment

creotutar Sep 3, 2020

Choose a reason for hiding this comment

alexisgrow left a comment

Choose a reason for hiding this comment

tmeyer2115 Sep 3, 2020

Choose a reason for hiding this comment

creotutar Sep 3, 2020

Choose a reason for hiding this comment

oshi97 left a comment

Choose a reason for hiding this comment

oshi97 Sep 3, 2020

Choose a reason for hiding this comment

creotutar Sep 4, 2020

Choose a reason for hiding this comment

creotutar commented Sep 3, 2020 •

edited

Loading

cea2aj Sep 3, 2020 •

edited

Loading