Combine VAudioDetails and VImageDetails into VMediaDetails component
#2846
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
openverse-bot
added
the
🚦 status: awaiting triage
|
Size Change: +1.44 kB (0%) Total Size: 880 kB
ℹ️ View Unchanged
|
obulat
added
🟨 priority: medium
obulat
force-pushed
the
simplify-media-details
branch
from
45d6b80 to
cbe8fa3
Compare
obulat
changed the title
VAudioDetails and VImageDetails into VMediaDetails component
dhruvkb
approved these changes
Aug 22, 2023
| return [...uniqueFormats].join(", ") | ||
| } | ||
|
|
||
| export const getMediaMetadata = ( |
There was a problem hiding this comment.
If this function is branched over image and audio it might be clearer if it called one of two separate functions depending on the media type.
There was a problem hiding this comment.
I've just converted what we were doing in the VAudioDetails and VImageDetails components to a util function here. It's improved (and synced between image and audio metadata) in the follow-up PR.
obulat
force-pushed
the
simplify-media-details
branch
2 times, most recently
from
7d98bf7 to
9042719
Compare
github-actions
bot
added
the
🧱 stack: documentation
|
Full-stack documentation: https://docs.openverse.org/_preview/2846 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. |
|
Based on the medium urgency of this PR, the following reviewers are being gently reminded to review this PR: @AetherUnbound Excluding weekend1 days, this PR was ready for review 4 day(s) ago. PRs labelled with medium urgency are expected to be reviewed within 4 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
|
obulat
force-pushed
the
simplify-media-details
branch
2 times, most recently
from
11d8e17 to
75b9bb5
Compare
obulat
force-pushed
the
simplify-media-details
branch
from
75b9bb5 to
29744f6
Compare
AetherUnbound
approved these changes
Aug 29, 2023
| type: Object as PropType<AudioDetail | ImageDetail>, | ||
| required: true, | ||
| }, | ||
| imageWidth: { |
There was a problem hiding this comment.
image here makes me think that this might be specific to image media, although I know that the album art is likely used here for audio records. Does maybe previewWidth or mediaWidth make more sense, to make this as generic as possible? Or should we stick with the image prefix?
There was a problem hiding this comment.
It's actually the property that's specific for the images. All of the audio thumbnails are square. Some images, on the other hand, don't have the values for width, height and image type in the API result. So, we send a head request to the provider, and update these values before displaying them. This is why the values are set separately here.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes
Related to #2731 by @fcoveram
Description
To make the updates to the metadata section of the single result page easier, this PR replaces the separate
VAudioDetailsandVImageDetailscomponents with a singleVMediaDetailscomponent. The metadata (source, provider, dimensions for images and sample rates etc for audio) are prepared in a new utility function inmetadata.ts. This function creates a list of the title i18n keys, values and urls (if available) for the media metadata such as source/creator/file size/file format etc.This PR also moves the tag filtering (to filter out empty tags) from the components to the
decodeMediaDatafunction that prepares each media item from the API response to be displayed on the frontend.Some changes to the
metadata.tsandVMetadatain this PR will be improved on in #2851. Here, the effort was made to extract the components without affecting the snapshots, as much as possibleTesting Instructions
Run the app and see that the single result metadata looks correct, and works both on the audio and image pages.
Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin