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
Use the VMediaCollection
for search and collection results
#3835
Conversation
80f4a61
to
a8b7f8c
Compare
01c709c
to
7b790d8
Compare
519cfee
to
fd3a46d
Compare
7b790d8
to
ea62aaa
Compare
ea62aaa
to
aae808e
Compare
20b6c6b
to
5381bb0
Compare
3db9e99
to
5e7c35a
Compare
This comment has been minimized.
This comment has been minimized.
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.
Before I dive too deeply into the code I am seeing two bugs. Let me know if this PR is the right place to address these; if not, I am happy to ignore them and re-review!
- There is a regression where the placeholder "skeleton" UI isn't shown when switching media types via the content switcher.
- When navigating to a source collection from a creator collection, the results are not re-fetched.
This should be addressed in this PR
Should be fixed in #3886 I'm going to draft this PR to address the first point. |
5e7c35a
to
1f81365
Compare
VMediaCollection
for search result for a better code organizationVMediaCollection
for search and collection results
It turns out there were too many
When I first started implementing this PR, we had no "source" link button on the creator page, so there was no way of navigating from one collection to another. You could only navigate from a collection to a single result. That's why I didn't even test this scenario. |
e264994
to
07088d5
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.
This LGTM with the caveat that there's a typo in the translation paths which I have outlined here. I will pre-approve though since that's only a typo. Everything is behaving well.
Signed-off-by: Olga Bulat <obulat@gmail.com>
Signed-off-by: Olga Bulat <obulat@gmail.com>
Signed-off-by: Olga Bulat <obulat@gmail.com>
Handle fetching in `collection.vue` page, and replace `VCollectionPage` with `VCollectionResults` Signed-off-by: Olga Bulat <obulat@gmail.com>
243991b
to
3c255a7
Compare
Signed-off-by: Olga Bulat <obulat@gmail.com>
Signed-off-by: Olga Bulat <obulat@gmail.com>
Signed-off-by: Olga Bulat <obulat@gmail.com>
Signed-off-by: Olga Bulat <obulat@gmail.com>
Signed-off-by: Olga Bulat <obulat@gmail.com>
f994f1a
to
d10d443
Compare
Full-stack documentation: https://docs.openverse.org/_preview/3835 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. |
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!
Fixes
Based on #3831
Fixes #3884 by @obulat
Fixes #3576 by @AetherUnbound
Fixes #3830 by @obulat
Initially, I split the changes into the search result and collection result changes. However, this made it more difficult to understand the scope and changes, and to test the PRs. This is why I merged the collection changes (as well as the deletion of the now-unused components) into this PR.
Description
This PR fixes the collection result fetching and improves the fetching of the search results.
It uses the new
VMediaCollection
component and 2 new components for the search and collection results.VSearchResults
component fills in the components necessary for search results: search title and content links in the header, load more button, and the external search form in the footer.VCollectionResults
component adds the components necessary for collection results:VCollectionHeader
and load more button.The media fetching was updated: now it is done in a single place:
search.vue
andcollection.vue
, instead of being split among these pages, middleware and the Load more component. This allows us to handle fetching, refetching, and loading more items from the same place. The "Load more" button now only handles sending the Analytics events.Testing Instructions
Run the app with
additional_search_views
on.Check that the search results are updated correctly: when the search term is updated, search type is changed, filters are updated or load more button is clicked. The result labels should be updated correctly, loading skeletons shown when media is fetching for the first time, media should be updated for the correct query.
The same check needs to be done for collection pages
The search results should be updated correctly when the
additional_search_views
is off.Repeat the instructions from #3884 and see that the problem is fixed and results are refreshed
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin