-
Notifications
You must be signed in to change notification settings - Fork 166
Add support for multiple content shares #998
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 |
---|---|---|
|
@@ -5,12 +5,16 @@ import { ContentShare } from './'; | |
|
||
# ContentShare | ||
|
||
The `ContentShare` component renders a `ContentTile` for the active content share video, remote or local. | ||
The `ContentShare` component renders a `ContentTile` for a content share video, remote or local. | ||
|
||
If used within the `VideoGrid` component, it will automatically place the active tile in the featured grid slot. It takes precedence over the featured video tile. | ||
|
||
Once a meeting session has been started, a user can start and stop content sharing by using the `useContentShareControls` hook. | ||
|
||
## Multiple Content Shares | ||
|
||
With the support for multiple content shares, you can now specify which content share tile to render by providing the `tileId` prop. If no `tileId` is provided, the component will render the default content share tile from (`const { tileId } = useContentShareState()`). | ||
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. So what will the 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. We should also link to 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.
I think this is clear enough. It will use the existing 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 clarify, |
||
|
||
## Importing | ||
|
||
```javascript | ||
|
@@ -19,26 +23,85 @@ import { ContentShare } from 'amazon-chime-sdk-component-library-react'; | |
|
||
## Usage | ||
|
||
### With Single Content Share | ||
|
||
```jsx | ||
import React from 'react'; | ||
import { | ||
MeetingProvider, | ||
ContentShare, | ||
useContentShareControls | ||
useContentShareControls, | ||
} from 'amazon-chime-sdk-component-library-react'; | ||
|
||
const App = () => { | ||
const { toggleContentShare } = useContentShareControls(); | ||
|
||
return ( | ||
<MeetingProvider> | ||
<ContentShare nameplate='Content share' /> | ||
<ContentShare nameplate="Content share" /> | ||
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. Are we using " or ' in this repo? 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. We use single quote for JS, but here it should be double quote because it's a property on JSX element. Like we should use double quote on HTML element |
||
<button onClick={toggleContentShare}>Toggle content share</button> | ||
</MeetingProvider> | ||
); | ||
}; | ||
``` | ||
|
||
### With Multiple Content Shares | ||
|
||
```jsx | ||
import React from 'react'; | ||
import { | ||
MeetingProvider, | ||
ContentShare, | ||
ContentShareProvider, | ||
useContentShareState, | ||
useContentShareControls, | ||
} from 'amazon-chime-sdk-component-library-react'; | ||
|
||
const App = () => { | ||
return ( | ||
<MeetingProvider> | ||
<ContentShareProvider maxContentShares={2}> | ||
<ContentShareView /> | ||
<ContentShareControls /> | ||
</ContentShareProvider> | ||
</MeetingProvider> | ||
); | ||
}; | ||
|
||
const ContentShareView = () => { | ||
const { tiles, tileIdToAttendeeId } = useContentShareState(); | ||
|
||
return ( | ||
<div> | ||
{tiles.length === 0 ? ( | ||
<p>No content is being shared</p> | ||
) : ( | ||
<div className="content-share-container"> | ||
{tiles.map((tileId) => ( | ||
<ContentShare | ||
key={tileId} | ||
tileId={tileId} | ||
nameplate={`Shared by: ${tileIdToAttendeeId[tileId.toString()]}`} | ||
/> | ||
))} | ||
</div> | ||
)} | ||
</div> | ||
); | ||
}; | ||
|
||
const ContentShareControls = () => { | ||
const { toggleContentShare } = useContentShareControls(); | ||
const { canStartContentShare } = useContentShareState(); | ||
|
||
return ( | ||
<button onClick={toggleContentShare} disabled={!canStartContentShare}> | ||
Share content | ||
</button> | ||
); | ||
}; | ||
``` | ||
|
||
## Props | ||
|
||
<ArgTypes of={ContentShare} /> |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -10,37 +10,49 @@ import { BaseSdkProps } from '../Base'; | |||
|
||||
interface Props extends BaseSdkProps { | ||||
nameplate?: string; | ||||
tileId?: number; | ||||
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. Note that since the For builders, they will need to create VideoTileGrid to handle multiple content share tiles based on their UX using updated |
||||
} | ||||
|
||||
export const ContentShare: React.FC<React.PropsWithChildren<Props>> = ({ | ||||
className, | ||||
tileId, | ||||
...rest | ||||
}) => { | ||||
const audioVideo = useAudioVideo(); | ||||
const { tileId } = useContentShareState(); | ||||
const contentShareState = useContentShareState(); | ||||
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. Thinking from keeping the props limited with no new optional props idea. How about we maintain 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. But yes, we would have to maintain support of 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. In short, I decide to let builder to decide how and where to render the multiple content share tiles via the new |
||||
|
||||
// Use the provided tileId or fall back to the default (for backward compatibility) | ||||
const tileIdToRender = | ||||
tileId !== undefined ? tileId : contentShareState.tileId; | ||||
|
||||
const attendeeId = tileIdToRender | ||||
? contentShareState.tileIdToAttendeeId[tileIdToRender.toString()] | ||||
: null; | ||||
|
||||
const videoEl = useRef<HTMLVideoElement | null>(null); | ||||
|
||||
useEffect(() => { | ||||
if (!audioVideo || !videoEl.current || !tileId) { | ||||
if (!audioVideo || !videoEl.current || !tileIdToRender) { | ||||
return; | ||||
} | ||||
|
||||
audioVideo.bindVideoElement(tileId, videoEl.current); | ||||
audioVideo.bindVideoElement(tileIdToRender, videoEl.current); | ||||
|
||||
return () => { | ||||
const tile = audioVideo.getVideoTile(tileId); | ||||
const tile = audioVideo.getVideoTile(tileIdToRender); | ||||
if (tile) { | ||||
audioVideo.unbindVideoElement(tileId); | ||||
audioVideo.unbindVideoElement(tileIdToRender); | ||||
} | ||||
}; | ||||
}, [audioVideo, tileId]); | ||||
}, [audioVideo, tileIdToRender]); | ||||
|
||||
return tileId ? ( | ||||
return tileIdToRender ? ( | ||||
<ContentTile | ||||
objectFit="contain" | ||||
className={className || ''} | ||||
className={`ch-content-share--${tileIdToRender} ${className || ''}`} | ||||
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. is this a defined class name? 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. No, this className is not used in fact, I added here to follow how we did for amazon-chime-sdk-component-library-react/src/components/sdk/RemoteVideo/index.tsx Line 50 in efb7f0f
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. The prefix classname is to make sure we can confidently override the styles on this one and is a way to let builders know that they should not change these I believe. |
||||
{...rest} | ||||
ref={videoEl} | ||||
data-content-share-attendee={attendeeId} | ||||
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. Is this for testing? 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. yeah, this is to make it easier for builder to know which tile is for which attendee so they can locate the element quickly. |
||||
/> | ||||
) : null; | ||||
}; | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,13 +6,27 @@ import { Meta } from '@storybook/blocks'; | |
|
||
The `ContentShareProvider` provides state and functionality for content sharing. | ||
|
||
### Props | ||
|
||
```javascript | ||
{ | ||
// Maximum number of concurrent content shares allowed (default: 1, range: 1-2) | ||
maxContentShares?: number; | ||
} | ||
``` | ||
|
||
### State | ||
|
||
```javascript | ||
{ | ||
// The tile ID of the active content share | ||
// The tile ID of the active content share (deprecated, maintained for backward compatibility) | ||
// When multiple content shares are present, this points to the most recently started content share | ||
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. If there is a task for next major version release, we should have a point to rename this to 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. No, we don't plan a major version release. When we do major version release in the future we will delete 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. Yes it seems now the active tiles will be under |
||
tileId: number | null; | ||
|
||
// The chime attendee ID of the user sharing (deprecated, maintained for backward compatibility) | ||
// When multiple content shares are present, this points to the attendee of the most recently started content share | ||
sharingAttendeeId: string | null; | ||
|
||
// Whether the content share is paused | ||
paused: boolean; | ||
|
||
|
@@ -22,8 +36,17 @@ The `ContentShareProvider` provides state and functionality for content sharing. | |
// Whether or not the local user's content share is loading | ||
isLocalShareLoading: boolean; | ||
|
||
// The chime attendee ID of the user sharing | ||
sharingAttendeeId: string | null; | ||
// Whether the user can start a content share based on current limits and `isLocalUserSharing` | ||
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. We can expand a bit more on "current limits" for clarity here. Is this just the 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. Yeah, let me update it, it should be 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. Seems like still needs an update? |
||
canStartContentShare: boolean; | ||
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. We might want to change this to 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. +1 for decouple, feel like state should be independent from each other here, also, builder may have feature on 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. My initial thought is to add a convenient boolean flag that builders can use to disable the content share button. This is needed to handle two scenarios:
For scenario 1, previously, with single content share, if a second attendee started sharing, the SDK would automatically stop the existing share (takeover). However, now that we support multiple (2) concurrent content shares, attempting a third share will fail due to media limits, and the takeover won't occur. Since error handling for this case isn't straightforward (the error can't be caught directly in the API call), it's better to proactively disable the content share button when limits are reached. Even without
It's a comparison between below three options, and I think option 1 is most convenient and aligns with my intention.
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. if there an use case that builder can implement to kick off the 1st sharing when the 3rd starts? In this case, even though it reaches the max limit, but it literally is allowed to start content share. 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. Today we don't allow three content shares from media side. So they cannot start a 3rd one, but they can stop one content share first, then 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. Hmm thinking option 1 is simplest for builders? Reason: The limit on whether a content share can start or not when its at max limit + local user sharing is decided by the library and not by the builders. If we add Thus, current props sounds fine to me. But we have to explain this in the documentation. 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. Cool, I'll go with option 1 now. We can always add |
||
|
||
// Array of all content share tile IDs | ||
tiles: number[]; | ||
|
||
// Map of tile IDs to attendee IDs | ||
tileIdToAttendeeId: { [key: string]: string }; | ||
|
||
// Map of attendee IDs to tile IDs | ||
attendeeIdToTileId: { [key: string]: number }; | ||
} | ||
``` | ||
|
||
|
@@ -53,6 +76,49 @@ const MyChild = () => { | |
}; | ||
``` | ||
|
||
## Multiple Content Shares | ||
|
||
You can configure the maximum number of concurrent content shares allowed (up to 2): | ||
|
||
```jsx | ||
import React from 'react'; | ||
import { | ||
MeetingProvider, | ||
ContentShareProvider, | ||
useContentShareState, | ||
} from 'amazon-chime-sdk-component-library-react'; | ||
|
||
const App = () => ( | ||
<MeetingProvider> | ||
{/* Override the default maxContentShares value (1) in ContentShareProvider */} | ||
<ContentShareProvider maxContentShares={2}> | ||
<MyComponent /> | ||
</ContentShareProvider> | ||
</MeetingProvider> | ||
); | ||
|
||
const MyComponent = () => { | ||
const { tiles, tileIdToAttendeeId, canStartContentShare } = | ||
useContentShareState(); | ||
|
||
return ( | ||
<div> | ||
<p>Content shares available: {tiles.length}</p> | ||
<p>Can start content share: {canStartContentShare ? 'Yes' : 'No'}</p> | ||
<div> | ||
{tiles.map((tileId) => ( | ||
<ContentShare | ||
key={tileId} | ||
tileId={tileId} | ||
nameplate={`Shared by: ${tileIdToAttendeeId[tileId.toString()]}`} | ||
/> | ||
))} | ||
</div> | ||
</div> | ||
); | ||
}; | ||
``` | ||
|
||
## Usage without MeetingProvider | ||
|
||
If you opt out of using `MeetingProvider`, you can drop in a `ContentShareProvider` and use its state. Make sure that its dependencies are rendered higher in the tree. | ||
|
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.
nonblocker:
maxConcurrentContentShare
may be a better name?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 feel 'concurrent' is redundant here, since 'maxContentShares' itself is already clear and unambiguous.
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.
What if we dont have such a prop? Can we take Video tiles like route. For example, we have a max limit on video streams but we dont have a prop for the max limit for them limiting builders. Correct me if not here. Have to check how that is limited component usage wise.
Uh oh!
There was an error while loading. Please reload this page.
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'm not following your question, could you explain a bit more?
We need this props for two reasons:
tiles.length
, and have a flag to disable the button when ittiles.length <=1
by themselves.Uh oh!
There was an error while loading. Please reload this page.
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.
Right, just trying to see whether we adding a new prop vs using the
tiles.length
is fine too. If the later is fine, why introduce a new one? Now still going through review, will understand and sync on the breaking change part. Are you saying that builders if today with existing library APIs try to start 2 content share (if they can), that would break content share components?Uh oh!
There was an error while loading. Please reload this page.
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.
Since this is a breaking change and we're not planning a major version release, we need to implement an opt-in mechanism so builders can explicitly enable this feature.
Before this change when the second attendee starts content share, sdk will stop existing content share (when it detects a second content share tile), so it ensures only one content share tile available. After this change, when allowing multiple content share, starting the second content share won't stop the existing one. This is the breaking behavior and user needs to explicitly opt-in this
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.
Right I get it now, its more like a feature flag in a way where its honored and backward compatible in the
ContentShareProvider
.