-
Notifications
You must be signed in to change notification settings - Fork 63
Conversation
e882a16
to
edc9a43
Compare
edc9a43
to
6d16616
Compare
This is great Olga! I haven't tested the code locally yet but I did take it for a spin to do some fun TypeScript hoop-jumping to make the MediaStore a generic class based on the |
Thank you, Sara, your PR is amazing! I've left my notes there, and added some changes from it here. |
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! Tested locally and it works great. Left a couple more subtle TypeScript things but given the files aren't added to tsconfig
yet anyway it's no big deal to leave them out, we'll just have to fix them later when we add them to tsconfig
.
Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com>
Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com>
@WordPress/openverse-frontend can we get a second review on this? 🙂 |
const getResourceSlug = (resource) => { | ||
const slug = { [AUDIO]: 'audio', [IMAGE]: 'images' }[resource] ?? resource | ||
return `${slug}/` | ||
} |
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.
nice fix here.
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.
The new abstraction is quite nice, and the types are fantastic. Some non-blocking questions:
- Should we add the
peaks
key here to theAudioDetail
interface? Since it will be used for the waveforms soon and is already accessed in theVWaveform
component implementations. - How will we handle if a media type has a one-off endpoint, like the
/waveform
endpoint for audio or future endpoints that, say, 3D models might need?
Subclass the class Model3DService extends MediaService<'3d models'> {
constructor() {
super('3d models')
}
getCustomData() {
// impl
}
} |
* Rename AudioDetails (#892) * Rename AudioDetails folder * Fix store access * Revert conversion to Composition API * Revert conversion to defineComponent * Update pot * Use v-show instead of v-if for width-based condition (#884) * Use v-show instead of v-if for width-based condition Use v-show instead of v-if to fix server-client mismatch * Fix waveform not loading on SSR * Split CI into discrete jobs (#981) * Split CI into discrete jobs * Update outdated POT file * Skip writing POT file if the only thing changing is the date * Format yaml * Add yaml to lint staged * Get locales when setting up node env * Cram it all into a single workflow * Add getting translations * Update license explanation tooltip (#850) Co-authored-by: Zack Krida <zackkrida@pm.me> Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com> * Fix attribution HTML glyph reference and fix historical usages as well (#990) * Fix attribution HTML glyph reference and fix historical usages as well * Add missing license parts * Add note to explain directory structure * Refactor media services (#867) * Create a single media-service data fetching object * Move slug creation from media service to the api service * Use typing data from #868 * Update src/constants/media.js Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com> * edit api-service styles; format * fix decode-media-data type import * Return decoded data from the media services Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com> * Make active media setup store and add unit tests * Commit the lock file * Fix merge problems Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com> Co-authored-by: Dhruv Bhanushali <dhruv_b@live.com> Co-authored-by: Zack Krida <zackkrida@pm.me>
* Install Pinia * Update the active media store to use Pinia * Delete the Vuex store * Update type definition and use it * Document the store composition variable * Fix a lint error that Prettier did not catch locally * Use `~` instead of `@` in imports * Update the nav store to use Pinia * Rename store * Make mutation payload fields non-nullable * Fix leftover incorrect usage of the active media store * Make active media setup store and add unit tests (#997) * Rename AudioDetails (#892) * Rename AudioDetails folder * Fix store access * Revert conversion to Composition API * Revert conversion to defineComponent * Update pot * Use v-show instead of v-if for width-based condition (#884) * Use v-show instead of v-if for width-based condition Use v-show instead of v-if to fix server-client mismatch * Fix waveform not loading on SSR * Split CI into discrete jobs (#981) * Split CI into discrete jobs * Update outdated POT file * Skip writing POT file if the only thing changing is the date * Format yaml * Add yaml to lint staged * Get locales when setting up node env * Cram it all into a single workflow * Add getting translations * Update license explanation tooltip (#850) Co-authored-by: Zack Krida <zackkrida@pm.me> Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com> * Fix attribution HTML glyph reference and fix historical usages as well (#990) * Fix attribution HTML glyph reference and fix historical usages as well * Add missing license parts * Add note to explain directory structure * Refactor media services (#867) * Create a single media-service data fetching object * Move slug creation from media service to the api service * Use typing data from #868 * Update src/constants/media.js Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com> * edit api-service styles; format * fix decode-media-data type import * Return decoded data from the media services Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com> * Make active media setup store and add unit tests * Commit the lock file * Fix merge problems Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com> Co-authored-by: Dhruv Bhanushali <dhruv_b@live.com> Co-authored-by: Zack Krida <zackkrida@pm.me> * fix possibly undefined * Add tests and other small fixes * Update nuxt.config.ts Co-authored-by: Zack Krida <zackkrida@pm.me> * Move converted modules to /stores/ and add unit tests * Fix linting error * Update nuxt.config.ts Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com> * Add changes from review * Register component * Make properties readonly * Remove dublicates Co-authored-by: Olga Bulat <obulat@gmail.com> Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com> Co-authored-by: Zack Krida <zackkrida@pm.me>
Fixes
Fixes #866 by @obulat
Description
This PR joins three separate modules for
media-service
s into one genericMediaService
that can be created with amediaType
parameter. All of the methods for bothaudio
andimage
are the same. This change will allow us to simplify media fetching, and make the store conversion to Pinia and addition of other media types easier.Testing Instructions
Run the app as usual, and check that you can search for all media types.
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin