feat: media player components#1097
feat: media player components#1097ethanwinters wants to merge 1 commit intocarbon-design-system:mainfrom
Conversation
✅ Deploy Preview for carbon-ai-chat-components ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for carbon-ai-chat-demo ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for ai-chat-components-react ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
ethanwinters
force-pushed
the
media-player-components
branch
2 times, most recently
from
1d6c4b4 to
23289c0
Compare
ethanwinters
force-pushed
the
media-player-components
branch
from
23289c0 to
36dea1d
Compare
davidmenendez
left a comment
davidmenendez
left a comment
There was a problem hiding this comment.
looks great! just a couple extremely minor things
| */ | ||
| render() { | ||
| const isSoundCloud = this.audioSourceType === AudioSource.SOUNDCLOUD; | ||
| const containerClass = `audio-player__container ${isSoundCloud ? "audio-player__container--soundcloud" : ""}`; |
There was a problem hiding this comment.
minor, but i think would benefit from classMap
| <div class="${containerClass}"> | ||
| ${this.hasError ? this.renderError() : ""} | ||
| <div | ||
| class="audio-player__provider ${this.hasError |
There was a problem hiding this comment.
minor, but i think would benefit from classMap
| * Internal state: expanded status | ||
| */ | ||
| @state() | ||
| private expanded = false; |
There was a problem hiding this comment.
more often then not i see requests for controlled state functionality from users. so i think we should either offer controlled and uncontrolled or just leave it to be controlled entirely by the user
| */ | ||
| private renderError() { | ||
| return html` | ||
| <div class="audio-player__error" role="alert" aria-live="assertive"> |
There was a problem hiding this comment.
minor, but i think we should include our class prefix across the classes
| const toggleLabel = `${actionLabel} ${displayLabel}`; | ||
|
|
||
| return html` | ||
| <div class="transcript"> |
There was a problem hiding this comment.
minor, but i think we should include our class prefix across the classes
2 participants
Closes #350
Add standalone audio and video player components with multi-provider support
Changelog
New
audio-playerweb component with support for native audio, SoundCloud URLs, and transcript displayvideo-playerweb component with support for native video, YouTube, Vimeo, and Kaltura providerstranscriptweb component for displaying audio transcriptsaudio-player,video-player, andtranscriptcomponentsChanged
MediaPlayer.tsxto use newaudio-playerandvideo-playercomponents instead of react-playerAudioComponent.tsxto integrate with new audio player componentRemoved
Testing / Reviewing
Storybook Testing:
npm run storybookin thepackages/ai-chat-componentsdirectoryDemo Site Testing:
npm run devfrom the root directoryAudio Testing Commands:
audio - soundcloud- Tests SoundCloud audio embeddingaudio - mp3- Tests native mp3 file with transcript functionalityVideo Testing Commands:
video - youtube- Tests YouTube video embeddingvideo - vimeo- Tests Vimeo video embeddingvideo - kaltura- Tests Kaltura video embedding (may fail due to domain restrictions)Functional Checklist:
audio - soundcloudcommand loads and plays SoundCloud audio correctlyaudio - mp3command loads native mp3 file and displays transcriptvideo - youtubecommand embeds and plays YouTube video correctlyvideo - vimeocommand embeds and plays Vimeo video correctlyvideo - kalturacommand attempts to load Kaltura video (expected to fail with domain restriction message)