-
Notifications
You must be signed in to change notification settings - Fork 212
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
Reset the media store state if collection state changes #3979
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.
I don't love having to manage isCollectionStateSame
as a custom approach to checking for deep equality of two objects, but I am not sure we have a simpler alternative at the moment and this appears to be working. LGTM! We should really move to using lodash or similar in the future IMO.
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 wanted to try and review this PR, but it doesn't have any testing instructions so I'm not sure how best to proceed 🙂 I do have one question.
frontend/src/stores/search.ts
Outdated
isCollectionStateSame( | ||
collectionParams: CollectionParams, | ||
mediaType: SupportedMediaType | ||
) { |
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.
Is there an easy way to test this new function?
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 removed this function in favor of @wordpress/is-shallow-equal that we are already using in another component.
Signed-off-by: Olga Bulat <[email protected]>
Signed-off-by: Olga Bulat <[email protected]>
8cb65e3
to
1a5de2c
Compare
Sorry, I updated the instructions :) |
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.
Perfect, thanks for the testing instructions! Glad we can use an existing function here too 😄
Fixes
Fixes #3976 by @zackkrida
Description
This PR adds
clearMedia
call to the search store's function setting the collection state. This makes thesetCollectionState
similar tosetSearchStateFromUrl
. To make sure that we don't clear media when non-collection state params change (for instance, if we add filters to the collection views, or if we addpage
parameter to the frontend URL to improve pagination), we first check if thecollectionParams
are different.Testing Instructions
Go through the following steps and check that the linked issue is fixed (step 5):
just frontend/run dev
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin