-
Notifications
You must be signed in to change notification settings - Fork 143
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
Filter link on search displays results #742
Conversation
Still need to write tests for this change |
packages/template-retail-react-app/app/pages/product-list/index.jsx
Outdated
Show resolved
Hide resolved
@@ -82,6 +82,20 @@ const MockedComponent = ({isLoading, isLoggedIn = false, searchQuery}) => { | |||
</div> | |||
)} | |||
/> | |||
<Route |
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.
There's no need to add a new <Route />
, we can add the '/search'
path to the existing <Route />
:
path={createPathWithDefaults('/category/:categoryId')} |
path={[
createPathWithDefaults('/category/:categoryId'),
createPathWithDefaults('/search')
]}
@@ -108,7 +122,6 @@ const server = setupMockServer( | |||
) | |||
|
|||
beforeEach(() => { | |||
window.history.pushState({}, 'ProductList', '/uk/en-GB/category/mens-clothing-jackets') |
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.
We can leave this pushState
by default for the majority of the tests.
I suggest reverting this change.
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.
Since we are adding tests for both routes /search and /category/:id for the PLP, having module-wise navigation to the category page for all tests does not make any sense for the one that needs to navigate to the search page. I think it is better for the individual tests to handle the URL navigation instead of having a global one for all tests.
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.
Yes, currently there are two tests that need to navigate to the search page. For potential future test cases, I also suggest having the individual tests handle the URL navigation!
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.
I can see advantages for both approaches, less code vs more specific code. Either way works great in this case.
Feel free to ignore the window.history.pushState
comments @yunakim714.
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.
Good job! I left only small comments.
CI is currently failing due to an ESLint issue, which you can fix by running npm run lint:fix
.
Feel free to merge once tests pass.
Co-authored-by: Adam Raya <[email protected]>
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.
LGTM
Description
Currently, when users search for an item and then apply a filter from the search results page, they are directed to a 404 error page that looks like this:
https://pwa-kit.mobify-storefront.com/en-GB/category/undefined?limit=25&q=dress&refine=c_refinementColor%3DBlack&sort=best-matches
This fix ensures that users can filter on search results.
Types of Changes
Changes
How to Test-Drive This PR
npm start
Checklists
General
Accessibility Compliance
You must check off all items in one of the follow two lists:
or...
Localization