-
Notifications
You must be signed in to change notification settings - Fork 63
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.
LGTM aside from the expect().rejects
issue I pointed out in a few places. Otherwise once those are addressed this will be a nice addition to the test suite 😁
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! 🚀
220170a
to
d47bc40
Compare
I also added a bug-fix in here: both SearchGridManualLoad and search page were watching the query and updating the media in store on change. It is enough to watch for query changes on the search page. To make sure that all changes in query trigger the watcher, I added the |
@obulat Now that there are some changes outside of adding tests can you add testing instructions? |
There was previously a bug when on a Search Form submit (clicking |
Add initial query values Handle media reset on query change only in `search` page Handle media reset on query change only in `search` page Remove unused lines Update src/utils/prepare-search-query-params.js Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com> Revert some unrelated changes Add check for newQuery
5c3e959
to
dadc62c
Compare
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! No worries about the back and forth 😅
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.
🥳
Fixes
Fixes #285 by @obulat
Description
This PR adds tests for
search-store
and raises the coverage to 100%.This PR also removes the actions for fetching collection images because they are not used anywhere. If needed, they can be re-added in the future.
To make adding media types easier, actions are created using an object now:
instead of
actions(audioService, imageService)
,they use a
services
object:services = { 'audio': audioService, 'image': imageService }
.A couple of small bugs were also fixed:
MEDIA_NOT_FOUND
is called correctly, using bothstate
andparams
as parameters.FETCH_AUDIO
, theusageSessionId
is set correctly, using the namespaceduser.usageSessionId
only theI've reverted this changesearch
page watches for query changes, and updates the media in Vuex store, instead of bothsearch
page andSearchGridManualLoad
.Testing Instructions
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin