search

Rebuild-Black-Business / RBB-Website

Website to help connect black-owned businesses with consumers and resources
https://www.rebuildblackbusiness.com/
MIT License
118 stars 72 forks source link

Don't merge without Discussion/Picking a solution! - Fixes going back to first page showing all results #248

Closed juanri0s closed 4 years ago

juanri0s commented 4 years ago

Using this PR for discussion on the below issue/solution:

https://trello.com/c/QNWA1y7N

Issue: A user searches for 'Portland', the url is https://www.rebuildblackbusiness.com/businesses/all?location=Portland. User then goes to page 2, the url is then businesses/all?location=Portland&page=2. If the user clicks the back arrow or clicks on the previous page the url then becomes https://www.rebuildblackbusiness.com/businesses/all causing all results to show even though you just wanted 'Portland'.

removing if (page === 1) return pathname; fixes a majority of the issue.

This line made it so that if it was the first page, return the path name without setting it based on the query params (the original url - https://www.rebuildblackbusiness.com/businesses/all). So removing this line will now show businesses/all?location=Portland&page=1 which is correct!

This helps us figure out the other part of the issue. When a user initially searches for 'Portland', the page param should be set to 1 in the url - businesses/all?location=Portland&page=1, this allows us to go back and forth between pages with no issues since the initial page is 1. This also gives us the benefit of having page 1 in the history so the user can navigate backwards if they want using their browser.

If having &page=1 doesn't look good/isn't a good solution, then we just need to make sure that when the page === 1 we do return ${pathname}?${getUpdatedSearchParams(location.search) without the page parameter which would require changes in getUpdatedSearchParams.

Both solutions would work it seems.

Open to other solutions/thoughts.

netlify[bot] commented 4 years ago

Deploy request for rebuild-black-business accepted.

Accepted with commit 640d01cb863267de86c45a30eef738e33b279f33

https://app.netlify.com/sites/rebuild-black-business/deploys/5ee33bb5366137000750b283

juanri0s commented 4 years ago

When changing search type to all from in need with predefined search parameters it just returns all results.

brentmclark commented 4 years ago

Great explanation of the issue and impact of the change!

Given how soon we launch, it might be wise to stick with the page=1 version. It's a low-risk fix that we can merge quickly. Doing so will maximize our opportunity to validate the change prior to launch.

brentmclark commented 4 years ago

Disregard my comments, another PR was made and merged already.

https://github.com/Rebuild-Black-Business/RBB-Website/pull/249/files

juanri0s commented 4 years ago

Will be closing this PR out now, thank you to everyone who reached out to discuss! @brentmclark I believe you initially pointed out the original source of the issue so thanks again! @llexical was able to implement some of the suggestions + more this morning and it looks like the solution worked.

Appreciate all the help on this one.