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
Combine related media component using VMediaCollection.vue
#3831
Conversation
5a7abcb
to
c7ff123
Compare
I'll review this in the next day! Somehow didn't see that I was on it. |
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 is the old VAudioCollection.vue
component.
f533da2
to
6bb70dd
Compare
? "audioDetails.relatedAudios" | ||
: "imageDetails.relatedImages" |
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.
Does this impact our line number matching in the .pot
files? I don't think so, because it's the full strings, but want to confirm.
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 know, will need to check. Would the line numbers make the strings fuzzy in GlotPress?
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 is really nice work. The type improvements and tests look good too. I left a minor suggestion and a design question.
Signed-off-by: Olga Bulat <obulat@gmail.com>
And also relatedTo to the related images
Signed-off-by: Olga Bulat <obulat@gmail.com>
Signed-off-by: Olga Bulat <obulat@gmail.com>
Co-authored-by: zack <6351754+zackkrida@users.noreply.github.com>
519cfee
to
fd3a46d
Compare
Based on the high urgency of this PR, the following reviewers are being gently reminded to review this PR: @dhruvkb Excluding weekend1 days, this PR was ready for review 5 day(s) ago. PRs labelled with high urgency are expected to be reviewed within 2 weekday(s)2. @obulat, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings. Footnotes
|
Fixes
Extracted from #3785 to make it easier to review.
Fixes #3832 by @obulat
Note: 428 lines of the 987 added lines are from "tapes" (saved API responses used for tests)
Description
We currently have two separate components for related images and related audio that are basically duplicating each other. This PR combines the 2 related components.
A bigger change is the new
VMediaCollection
component (originally introduced in #3785, but split from it to make reviewing simpler).This component handles displaying the header, grid skeleton if fetching or the media collection if fetched, and the footer with load more button and the external search form, if necessary.
To simplify this component, I used slots for header and footer, and created
wrapper
components (similar to the audio templates) for the related, search and collection results. This PR contains theVMediaCollection
and theVRelatedMedia.vue
wrapper component. It does not use footer because the related media does not have load more button (or the external search form).This PR renames the "old" components for audio and image grid to minimize the number of changes (
VImageGrid
andVAudioList
are "old", andVImageCollection
andVAudioCollection
are the new ones).There's also a tiny unrelated change: the loading skeleton for image single result is now centered (using
mx-auto
class).Testing Instructions
Select an audio result and click on one of the related audios. You should see the
SELECT_RESULT
event with therelatedTo
set correctly. The related images/audio should look correct.Look at the single result pages. The related media section should look and behave correctly.
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin