Participant Item to show Participant States#2161
Conversation
…ttps://github.com/Azure/communication-ui-library into anjulgarg/2940249-videotile-call-connection-state
…ttps://github.com/Azure/communication-ui-library into anjulgarg/2940468-participant-state
…com/Azure/communication-ui-library into anjulgarg/2940251-localise-videotile-participantState
|
Failed to pass the composite UI Test. If this PR is for UI change and the error is snapshot mismatch, please add "ui change" label to the PR for updating the snapshot. |
…ttps://github.com/Azure/communication-ui-library into anjulgarg/2941671-participantState-participantItem
|
Failed to pass the UI Test. If this PR is for UI change and the error is snapshot mismatch, please add "ui change" label to the PR for updating the snapshot. |
|
LGTM except for @JamesBurnside point about a11y on desktop for keyboard users. |
| /* @conditional-compile-remove(one-to-n-calling) */ | ||
| /* @conditional-compile-remove(PSTN-calls) */ | ||
| /** String shown when `participantState` is `Ringing` */ | ||
| participantStateRinging?: string; |
There was a problem hiding this comment.
ideally this component is agnostic of being chat or calling and (to keep simple) also have props that do one and only one thing...
Thinking out loud so feel free to shut this down - Instead of passing participantState into here, would it be possible to pass any string that is simply written to the right hand side of the participant item, participantItemSecondaryText?: string - then the VideoGalleryParticipant does the mapping of state->string and passes in the appropriate string
There was a problem hiding this comment.
I understand your point of view. And it's a good idea.
However, that removes the localization support we offer for these connection states.
There's another concern of customer injected strings being too long and messing up the UI. They would then also have to deal with truncation.
By adding a fixed set of connection states, we keep this component communication specific.
There was a problem hiding this comment.
To keep the locale strings, would the localization not live in the videoGallery component locale?
There was a problem hiding this comment.
I think we need to handle truncation regardless and show an ellipsis or something for long text as we don't know what our (or Contoso's) translated strings will be, or what width Contoso will set the participantItem to be
There was a problem hiding this comment.
But does that mean that now we have to introduce these strings to VideoGallery props as well?
There was a problem hiding this comment.
They would be introduced there *instead (they should only need to be in one place, but I haven't dug into the code)
There was a problem hiding this comment.
We chatted offline and making this non-blocking for cost to implement this change.. but I still strongly feel this should happen, for posterity in case someone does decide to make this change reiterating why:
- Not pollute participantItem with calling-only properties
- Keep the property as simple as possible to ensure ease of use and understanding by Contoso devs (nothing surprising can happen like hiding the menu button)
- Keeps it flexible - e.g. if Contoso wanted "sharing screen" text here or something they could easily
|
Failed to pass the UI Test. If this PR is for UI change and the error is snapshot mismatch, please add "ui change" label to the PR for updating the snapshot. |
…ttps://github.com/Azure/communication-ui-library into anjulgarg/2941671-participantState-participantItem
…ttps://github.com/Azure/communication-ui-library into anjulgarg/2941671-participantState-participantItem
|
|
||
| {/* When the participantStateString has a value, we don't show the menu */} | ||
| {!me && participantStateString ? ( | ||
| <Text>{participantStateString}</Text> |
There was a problem hiding this comment.
do we have styles to handle truncation of long strings of text here?
There was a problem hiding this comment.
Can this be a work item under the feature somewhere (otherwise it will be a day-zero bug)
There was a problem hiding this comment.
Yes, going to bring this up in front of AlexP tomorrow.
Creating an item for this.
https://skype.visualstudio.com/SPOOL/_workitems/edit/2946830
| /** String shown when `participantState` is `Connecting` */ | ||
| participantStateConnecting?: string; | ||
| /* @conditional-compile-remove(one-to-n-calling) */ | ||
| /* @conditional-compile-remove(PSTN-calls) */ |
There was a problem hiding this comment.
how do double conditional compiled work? Is this an AND or an OR?
My assumption is the implementation will be an AND (both of these features must be stabilized for this feature to be public), but we would want an OR (if either of these features are stabilized we want this feature public)
There was a problem hiding this comment.
So if either of these feature is stabilized, we remove both the flags
Because the assumption is that if this is stable for one feature, it is stable for another as well
There was a problem hiding this comment.
We chatted offline, we don't have support for this so whoever stabilizes this feature first needs to ensure they delete both
There was a problem hiding this comment.
Filed a bug for tracking this Bug 2946816: [Web] We do not support OR conditional compilation flags
anjulgarg
deleted the
anjulgarg/2941671-participantState-participantItem
branch
5 participants
What
Uses consistent types b/w VideoTile and ParticipantItem participantState
Displays Participants state in participant item
FIGMA - https://www.figma.com/file/72qJ03EOQExf12g5JTdWjQ/Web-UI-Library?node-id=6180%3A48173
Why
How Tested
Process & policy checklist
Is this a breaking change?