-
Notifications
You must be signed in to change notification settings - Fork 63
Update search term when navigating with browser`s back and forward buttons #2101
Conversation
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.
This looks really good, @anton202! Do you think you could add an e2e test?
@obulat sure |
i dont know why the e2e/homepage.spec.ts test is failing... i didnt changed anything related with that test. but anyway, i`ve changed the css selector of the search type pop over, in that test and now it dose pass. regarding the e2e/translation-banner.spec.ts |
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.
It works beautifully, @anton202! And the tests are 💯
One request I have is to change the searchTerm
to be a prop for VImageCellSquare
/VAudioTrack
/ VImageCell
, and actually get it from the searchStore
in the parent components. This way, the searchStore.searchTerm
will be called once for each of the media items, and the Cell
/Track
components will not need to import the useSearchStore
.
sure, you rigth :) |
* # go back to single result page of cat | ||
* # search term inside the header should be set to cat | ||
*/ | ||
test("search term should be set to the search query param on an image type single result page", async ({ |
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.
Awesome test! It currently fails in staging :)
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.
so, does this test pass now?
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.
Awesome work, thank you, @anton202! Thank you for adding the changes and the test for them!
I've rebased your branch onto main and force-pushed it, so now the CI is green.
@obulat Thank you :) |
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.
This appears to be working perfectly. Great work @anton202
Fixes
Fixes #2082 by @obulat
Description
implemented the steps that described in the issue:
now, when doing a search from a single result page and then going back with the browser button
the search term in the header will be set correctly.
The curent State of staging
current-state-of-staging.mp4
This pull request
searc-term-pr.mp4
Testing Instructions
Follow the steps in the video :)
also, try this steps with an audio result
Checklist
Update index.md
).main
) ora parent feature branch.
errors.
Developer Certificate of Origin
Developer Certificate of Origin