Skip to content
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

Fix to make browser back button navigate pagination properly when faceted search is enabled #1606

Merged

Conversation

bc-williamkwon
Copy link
Contributor

@bc-williamkwon bc-williamkwon commented Dec 3, 2019

What?

Tested on store: https://williamkwon1576004500-testly-the-third.my-integration.zone/
Previously, the browser back button would not navigate pagination properly when faceted search was enabled but now it does.

Tickets / Documentation

Add links to any relevant tickets and documentation.

Screenshots (if appropriate)
https://imgur.com/a/MtmIeIR

@bigbot
Copy link

bigbot commented Dec 3, 2019

Autotagging @bigcommerce/storefront-team @davidchin

@mattolson
Copy link
Contributor

Please provide proof that the change works as expected.

@bc-williamkwon bc-williamkwon force-pushed the fixFacetedSearchPagination branch 8 times, most recently from 69cc26e to f0c6719 Compare December 4, 2019 23:25
@bc-williamkwon bc-williamkwon changed the title Fix pagination for when faceted search is enabled Fix to make browser back button navigate pagination properly when faceted search is enabled Dec 5, 2019
@bc-williamkwon bc-williamkwon force-pushed the fixFacetedSearchPagination branch 2 times, most recently from adc1b86 to bdd51cb Compare December 5, 2019 00:32
@bc-williamkwon
Copy link
Contributor Author

bc-williamkwon commented Dec 5, 2019

Added a screenshot and linked to store with theme that has changes applied to store.

mattolson
mattolson previously approved these changes Dec 5, 2019
@bookernath
Copy link
Contributor

I see the issue is still happening with a filter applied, e.g.:

https://williamkwon1575504304-testsworthy.my-integration.zone/shop-all/?_bc_fsnf=1&brand=36&limit=2

From here, you can navigate to page 2 and then click the back button - you'll notice the page number in the content footer does not change, and the products do not update.

ncheikh
ncheikh previously requested changes Dec 5, 2019
Copy link
Contributor

@ncheikh ncheikh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are testing and this does not appear to be working, we are digging in now, I will circle with you @bc-williamkwon.

It looks like it does not add the page query param to when:
Case 1:

  • Add Search Facet
  • Go to Page 2
  • Go Back via back button
  • Still on page 2

Case 2:

  • Go to Page 2
  • Go Back via back button
  • Still on page 2

Looks like the xhr request is not being fired when we go back

@mattolson mattolson dismissed their stale review December 5, 2019 19:59

Needs further testing

@bc-williamkwon bc-williamkwon force-pushed the fixFacetedSearchPagination branch 6 times, most recently from 49169c2 to 2541a6f Compare December 5, 2019 22:43
@bc-williamkwon
Copy link
Contributor Author

bc-williamkwon commented Dec 5, 2019

Made changes to trigger xhr request on back button click and check for page in query string.

@bookernath
Copy link
Contributor

The issue with the page not reloading seems fixed now.

However, it appears that we've introduced a new bug with this change:

Now if I'm on page 1 and I directly click on page 3, and then click the back button, I'll be taken to page 2, even though I never visited that page so it shouldn't be in my history.

@bc-williamkwon
Copy link
Contributor Author

Fixed said bug described in last comment

@bc-williamkwon bc-williamkwon force-pushed the fixFacetedSearchPagination branch 2 times, most recently from faf6e9b to 0c065d7 Compare December 6, 2019 19:53
@bc-williamkwon bc-williamkwon force-pushed the fixFacetedSearchPagination branch 2 times, most recently from 37e48a4 to 5a10392 Compare December 9, 2019 22:41
@bc-williamkwon
Copy link
Contributor Author

bc-williamkwon commented Dec 9, 2019

Made change to only update page parameter. The default page has a sort by query string parameter so I believe that it should be fine to assume that the url string has a query parameter.

@bc-williamkwon
Copy link
Contributor Author

Updated solution to use regex instead of split.

@bookernath
Copy link
Contributor

I've tested all the cases I tested before, and this looks good now 💚

@mattolson mattolson merged commit f14b572 into bigcommerce:master Dec 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants