-
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.
I think we still do need to add format filter on the front end as well for correctly handling duplicate alt_files. Something like this in the fullLayout getFormats can work
const getFormats = (audio) => {
if (audio.alt_files?.length) {
let extensions = new Set()
const filteredAltFiles = audio.alt_files.filter((altFile) => {
const isUnique =
altFile.filetype !== audio.filetype &&
!extensions.has(altFile.filetype)
extensions.add(altFile.filetype)
return isUnique
})
if (filteredAltFiles.length) {
return filteredAltFiles.map((altFile) => ({
extension_name: displayFormat(audio.provider, altFile.filetype),
download_url: altFile.url,
}))
}
}
return [
{
extension_name: displayFormat(audio.provider, audio.filetype),
download_url: audio.url,
},
]
}
@obulat For the duplicate formats are they the various mp3 bitrates that some providers have? In that sense they're not "duplicates", we're just missing information that should be encoded in them. I thought we'd discussed mapping this by hand on the frontend somewhere but I can't remember when that conversation happened or if an issue ever was created for it. |
FWIW I recall this too. |
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, fixed the dropdown icon issue which is the only part in scope for this.
Let's open a separate issue/continue the discussion about how to handle download file types elsewhere.
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!
I thought about the different bit rates too and it seems that those are duplicated in the catalog/from the API: https://api.openverse.engineering/v1/audio/9af3243a-e4d4-4254-95d9-a3d53a0158c3/ Response
So hiding it here is not the final solution although we could do it while solving the ingestion step, but I prefer to tackle that in the catalog first. |
Fixes
Fixes #1180 by @zackkrida
Description
This PR:
g
container elementV
prefixTesting Instructions
Visit the following link and observe that the button has the caret icon:
http://localhost:8443/audio/9af3243a-e4d4-4254-95d9-a3d53a0158c3
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin