Improve PreJoin Component and add custom participant placeholders #1129
Conversation
Feat/pre join improvements
🦋 Changeset detectedLatest commit: 8a0c30c The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
|
There was a problem hiding this comment.
hi @Simba14
thanks for taking the time to suggest and implement these changes.
It's clear that you've put a lot of thought into it.
I have some reservations about some of these changes though.
I tried to outline my concerns in some comments, feel free to correct my assumptions if any sound wrong to you.
The PR itself is (very) big and touches upon a bunch of things at the same time. We can keep this PR open to discuss your proposed changes. Whatever changes remain it would be nice to have them separated in PRs with smaller scope.
| * ``` | ||
| * @public | ||
| */ | ||
| export function useSelectedDevice<T extends LocalVideoTrack | LocalAudioTrack>({ |
There was a problem hiding this comment.
within the livekit-client SDK there's the concept of an active device which is currently used by the user.
That's not as useful for prejoin as of now, but it would mirror the implementation in the client SDK more closely.
| }; | ||
| }, [JSON.stringify(options, roomOptionsStringifyReplacer), onError, trackLock]); | ||
| // Handle audio track | ||
| React.useEffect(() => { |
There was a problem hiding this comment.
the way it was previously implemented was in order to avoid separate permission prompts when entering the site.
With your proposed implementation it would result in two different permission requests.
I don't think that's an improvement worth the additional complexity.
| /> | ||
| <div className="lk-button-group-menu-pre-join"> | ||
| <label className="lk-selected-device-label"> | ||
| {selectedAudioDevice?.label || micLabel} |
There was a problem hiding this comment.
this is a nice idea, but the handling could be moved into the instead in order to allow for the same experience everywhere (not only in the prejoin).
Keeping it simple with a useDeviceInfo(track) hook that only returns the MediaDeviceInfo for that use case seems more appropriate
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "name": "@livekit/components-styles", | |||
| "version": "1.1.4", | |||
| "version": "1.1.5", | |||
There was a problem hiding this comment.
versions are automatically bumped by the CI, bumping them in PRs has the effect of skipping versions or unintentionally publishing them
| display: flex; | ||
| flex-wrap: nowrap; | ||
| display: grid; | ||
| grid-template-columns: repeat(2, minmax(0, 1fr)); |
There was a problem hiding this comment.
these style changes aren't clear to me, why are they needed?
| @@ -73,6 +73,7 @@ export interface ParticipantTileProps extends React.HTMLAttributes<HTMLDivElemen | |||
| disableSpeakingIndicator?: boolean; | |||
|
|
|||
| onParticipantClick?: (event: ParticipantClickEvent) => void; | |||
| placeholders?: { [index: string]: React.ReactNode }; | |||
There was a problem hiding this comment.
not a big fan of passing the whole map to each participant tile.
A pattern where the component can take a single placeholder as a ReactNode seems preferable to me.
In general the codebase tries to avoid passing of components as props and rather works with slottable components (as children). We're already using those for ParticipantTile as custom tile renderings though, so I'm not totally against an exception here.
This PR improves the
PreJoincomponent by fixing a few bugs, improving the UX and adding some new features.Bug fixes:
Feature:
ParticipantTileby adding the placeholders prop. This allows placeholders to be unique for each participant e.g. if they have an image associated with their account.placeholdersis an object where the key is the participant id and the value is a React node.In order to implement the selected device feature, I added new hook -
useSelectedDeviceto return the current selected device by the user.