-
Notifications
You must be signed in to change notification settings - Fork 63
Add optional peaks key to AudioDetail interface #998
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.
Thank you so much for the contribution, and welcome to Openverse! Once you've updated the type to number[]
this should be ready for approving 🎉
src/store/types.ts
Outdated
@@ -97,6 +97,7 @@ export interface AudioDetail extends BaseMediaDetail<'audio'> { | |||
sample_rate?: number | |||
alt_files?: any | |||
filetype?: string | |||
peaks?: any |
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.
We can actually set this to number[]
specifically 🙂
peaks?: any | |
peaks?: number[] |
It might also be always defined, in which case we could remove the ?
but I'm not confident about that... and it's better safe than sorry when working with API responses anyway.
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.
@ChrisCoastal just wanted to make sure you saw this review. Thanks for your contribution so far!
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.
Hello @sarayourfriend and @zackkrida and thank you for the feedback and welcome to Openverse! I am very thankful for the guidance and opportunity. Apologies, as I've just seen this followup now, so I will changes and submit. Thanks!
Updated type expected by peaks key in AudioDetail interface to a number array.
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.
Thanks!
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.
Awesome, thank you! 🎉
@zackkrida do you feel comfortable if we merge this before the deployment? It's just a type change so as long as CI type check is passing we should be good. Feel free to merge if you're comfortable with it. |
Fixes
Fixes #996 by @obulat
Description
Adds optional peaks key to AudioDetail interface which will be soon used for the waveform. Type set to 'any'.
Testing Instructions
No testing written.
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin