-
Notifications
You must be signed in to change notification settings - Fork 131
Allow localtrack passing on BarVisualizer #1222
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@livekit/components-react": patch | ||
| --- | ||
|
|
||
| allow localtrack passing on bar visualizer |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,10 @@ | ||
| import * as React from 'react'; | ||
| import { useBarAnimator } from './animators/useBarAnimator'; | ||
| import { useMultibandTrackVolume, type AgentState } from '../../hooks'; | ||
| import type { TrackReferenceOrPlaceholder } from '@livekit/components-core'; | ||
| import { type TrackReferenceOrPlaceholder } from '@livekit/components-core'; | ||
| import { useMaybeTrackRefContext } from '../../context'; | ||
| import { cloneSingleChild, mergeProps } from '../../utils'; | ||
| import { LocalAudioTrack, RemoteAudioTrack } from 'livekit-client'; | ||
|
|
||
| /** | ||
| * @beta | ||
|
|
@@ -23,7 +24,9 @@ export interface BarVisualizerProps extends React.HTMLProps<HTMLDivElement> { | |
| state?: AgentState; | ||
| /** Number of bars that show up in the visualizer */ | ||
| barCount?: number; | ||
| /** @deprecated use `track` field instead */ | ||
| trackRef?: TrackReferenceOrPlaceholder; | ||
| track?: TrackReferenceOrPlaceholder | LocalAudioTrack | RemoteAudioTrack; | ||
|
Comment on lines
+27
to
+29
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: Other components currently seem to favor the What about instead, making
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's somewhat cleaner that way, true. But the downside of that is that the props wouldn't be as easy to inspect anymore, right? I thought deprecating one of them is pretty clear?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be clear I don't really have a strong opinion on this, it just struck me reading this code that it would be nice if they were mutually exclusive since they really should never both be set and then once you've migrated over to the new By "props wouldn't be as easy to inspect anymore" if you mean that the component props wouldn't point to a singular interface on a "go to definition" type operation and instead would be an expression of some sort - yes, that is a good point. I guess there are pros and cons to both approaches, so do whatever you think makes sense. |
||
| options?: BarVisualizerOptions; | ||
| /** The template component to be used in the visualizer. */ | ||
| children?: React.ReactNode; | ||
|
|
@@ -106,17 +109,18 @@ const getSequencerInterval = ( | |
| */ | ||
| export const BarVisualizer = /* @__PURE__ */ React.forwardRef<HTMLDivElement, BarVisualizerProps>( | ||
| function BarVisualizer( | ||
| { state, options, barCount = 15, trackRef, children, ...props }: BarVisualizerProps, | ||
| { state, options, barCount = 15, trackRef, track, children, ...props }: BarVisualizerProps, | ||
| ref, | ||
| ) { | ||
| const elementProps = mergeProps(props, { className: 'lk-audio-bar-visualizer' }); | ||
| let trackReference = useMaybeTrackRefContext(); | ||
| let targetTrack: TrackReferenceOrPlaceholder | LocalAudioTrack | RemoteAudioTrack | undefined = | ||
| useMaybeTrackRefContext(); | ||
|
|
||
| if (trackRef) { | ||
| trackReference = trackRef; | ||
| if (trackRef || track) { | ||
| targetTrack = trackRef || track; | ||
| } | ||
|
|
||
| const volumeBands = useMultibandTrackVolume(trackReference, { | ||
| const volumeBands = useMultibandTrackVolume(targetTrack, { | ||
| bands: barCount, | ||
| loPass: 100, | ||
| hiPass: 200, | ||
|
|
||
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.
thought: It looks like some changes from #1207 didn;t get included in this file - sorry about that, my bad 😞
However, I am a bit suprised that the ci step didn't check this? Maybe this is something to make a ticket on to look into, since it seems like this file isn't being checked like the associated
components-reactone is.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 don't check the API compat on CI for the core module as that's treated as an internal package.
Don't have a strong opinion on whether or not to change that.
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.
Ah, I see what you are saying, I guess that makes a certain amount of sense. Thinking about this in another way then - is there a good reason to even have the api-extractor tool run on the
corepackage and generate this artifact if it won't be consumed externally?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.
yeah, good point, both options sound fine to me!