Skip to content
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

Video 10788 track switch off UI #745

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

olipyskoty
Copy link
Contributor

Contributing to Twilio

All third party contributors acknowledge that any contributions they provide will be made under the same open source license that the open source project is provided under.

  • I acknowledge that all my contributions will be made under the project's license.

Pull Request Details

JIRA link(s):

Description

This PR updates the track switch off UI for v2 rooms according to the new figma designs. See PR #731 for the old designs where we blurred the video.

switchoffUI

Burndown

Before review

  • Updated CHANGELOG.md if necessary
  • Added unit tests if necessary
  • Updated affected documentation
  • Verified locally with npm test
  • Manually sanity tested running locally
  • Included screenshot as PR comment (if needed)
  • Ready for review

Before merge

  • Got one or more +1s
  • Re-tested if necessary

@olipyskoty olipyskoty mentioned this pull request Aug 30, 2022
10 tasks
@olipyskoty olipyskoty requested a review from timmydoza August 30, 2022 21:59
Copy link
Contributor

@timmydoza timmydoza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! We may want to try it out in a real room with a throttled connection.

Also, I noticed that the transitions aren't quite right when a participant is made the dominant speaker:
switch-off-transition
Here, "test1" is already showing the "Low Bandwidth" message in the sidebar, but as they move to the dominant speaker we see the fade to black transition again. I'm not exactly sure what the fix here would be but we can chat about it.

isSwitchedOff: {
opacity: 1,
visibility: 'visible',
transition: 'all 0.5s linear 2s',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about the 2s delay. This would display a frozen video for 2 seconds before it fades to black. Seems a little long. I'm not sure I like it because it means that the UI allows the user to be confused about the frozen video for 2 seconds before we reveal what is going on. I think we might just want to let the user know right away. A fade could still be fine though.

@@ -173,7 +189,17 @@ export default function MainParticipantInfo({ participant, children }: MainParti
</Tooltip>
)}
</div>
{(!isVideoEnabled || isVideoSwitchedOff) && (
<div
className={clsx(classes.avatarContainer, classes.trackSwitchOffContainer, {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if avatarContainer could be renamed to something more generic since it's now being used to contain something that isn't an avatar.

@olipyskoty olipyskoty requested a review from timmydoza September 7, 2022 21:37
Comment on lines 112 to 121
trackSwitchOffContainer: {
opacity: 0,
visibility: 'hidden',
transition: 'all 0.25s cubic-bezier(0.22, 0.61, 0.36, 1)',
},
isSwitchedOff: {
opacity: 1,
visibility: 'visible',
transition: 'all 0.5s linear',
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
trackSwitchOffContainer: {
opacity: 0,
visibility: 'hidden',
transition: 'all 0.25s cubic-bezier(0.22, 0.61, 0.36, 1)',
},
isSwitchedOff: {
opacity: 1,
visibility: 'visible',
transition: 'all 0.5s linear',
},

Don't think these are needed. Otherwise looks good!

@olipyskoty olipyskoty requested a review from timmydoza September 7, 2022 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants