Skip to content

Video 9513 update disabled video UI #708

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

Closed
wants to merge 18 commits into 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 blurs a participant's video whenever their video is switched off. After two seconds, a short message is displayed over the blurred video letting the user know that the video has been switched off to conserve bandwidth. This is so there isn't a quick flash of the message if the video is only switched off for a brief moment.

Also, I've updated the storybook controls so that you can unpublish/publish and switch on/switch off the video tracks individually, or a specific list of fake participants. This is the same behavior as the screenshare storybook controls.

Finally, comparing this branch against the grid view feature branch since main does not have storybook.

9513

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

timmydoza and others added 15 commits March 8, 2022 13:16
* Install storybook and add basic files

* Get storybook working

* Get grid view working

* Hook up menu button

* Fix some issues

* Fix index.html caching issue

* Update publication component and tests

* Update Room tests

* Update constants

* Upgrade typescript

* Update menu tests

* Update TS things for new version

* Rename state variable

* Add test for ParticipantAudioTracks

* Remove unused passcode stuff from twilio mock

* Automatically disable conversations in storybook

* Fix tests and linting issues

* Add tests for GridView

* Fix participant issue in stories

* Add comment for ParticipantAudioTracks

* Remove unnecessary code
* Install storybook and add basic files

* Get storybook working

* Get grid view working

* Hook up menu button

* Fix some issues

* Fix index.html caching issue

* Update publication component and tests

* Update Room tests

* Update constants

* Upgrade typescript

* Update menu tests

* Update TS things for new version

* Rename state variable

* Add test for ParticipantAudioTracks

* Remove unused passcode stuff from twilio mock

* Automatically disable conversations in storybook

* Fix tests and linting issues

* Add tests for GridView

* Install mui pagination component

* Fix issue with storybook controls

* Add pagination controls to state

* Create usePagination hook

* Add pagination to GridView component

* Add setting for MaxGridParticipants

* Remove unused constant and fix a few glitches

* Add tests for usePagination

* Update tests for GridView component

* Fix linting issue

* Only render Pagination component when there is more than one page

* Display participant count in menu

* Fix tests

* remove console log

* Fix linter errors

* Fix test language

* Fix issue with chat window rendering incorrectly
* Add useLocalStorageState hook

* Fix mocked localStorage class

* Add useLocalStorageState hook to app to persist settings

* Add comment to useLocalStorageState hook

* Update formatting
* Fix linter error

* Update code formatting
* Update node version in app.yaml

* Fix bug with usePresentationParticipants
@olipyskoty olipyskoty requested a review from timmydoza June 22, 2022 22:53
Comment on lines +115 to +123
'@keyframes showMessage': {
'0%': {
opacity: 0,
visibility: 'hidden',
},
'100%': {
opacity: 1,
visibility: 'visible',
},
Copy link
Contributor

Choose a reason for hiding this comment

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

With something like this, we can give the message a nice fade in:

'@keyframes showMessage': {
      '0%': {
        opacity: 0,
        visibility: 'hidden',
      },
      '80%': {
        opacity: 1,
        visibility: 'visible',
      },
      '100%': {
        opacity: 1
      },
    }

No need to use these exact values. Play with it and see what works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the idea of fading the message in, but I'm not sure I'm seeing much of a difference in the UI using your example. I tried changing opacity to something like 0.8 for 80% and that looked kind of glitchy. I did notice that increasing the animationDuration from 0.5s to 1s created a fade in effect. I'm wondering if you're seeing something that I'm not seeing though, so you might need to show me?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I wasn't originally seeing a fade in, but now I am. Not sure what changed, but it looks good now 👍

@@ -30,6 +30,7 @@ const useStyles = makeStyles((theme: Theme) =>
marginBottom: '0.5em',
'& video': {
objectFit: 'contain !important',
filter: 'none',
Copy link
Contributor

Choose a reason for hiding this comment

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

We can also transition the blur here. Again, try playing with the different values to see what works well. I liked this:

        transition: 'filter 1s cubic-bezier(0.22, 0.61, 0.36, 1)',

The cubic-bezier nonsense came from Chrome's devtools. There's a neat tool to play with easing functions:
image

@@ -121,6 +134,28 @@ const useStyles = makeStyles((theme: Theme) =>
fontSize: '0.75rem',
},
},
switchedOffMessage: {
Copy link
Contributor

Choose a reason for hiding this comment

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

This text could also have a shadow to make it more readable. Without it, you wouldn't be able to read white text on a white background. I don't know much about text-shadow, but something like this seemed ok: textShadow: '0 0 3px rgba(0, 0, 0, 0.7)',

More info here: https://css-tricks.com/almanac/properties/t/text-shadow/

@@ -29,7 +29,8 @@ const useStyles = makeStyles((theme: Theme) =>
overflow: 'hidden',
marginBottom: '0.5em',
'& video': {
objectFit: 'contain !important',
filter: 'none',
transition: 'filter 1s cubic-bezier(0.22, 0.61, 0.36, 1)',
Copy link
Contributor

Choose a reason for hiding this comment

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

Just make sure that this makes it into MainParticipantInfo too.

},
blur: {
'& video': {
filter: 'blur(10px)',
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the blur might be a little too much here. I feel like 4 or 5 px could be good for this component. Using a placeholder image a person, it seems a little too blurred:
image

10px might be good for MainParticipantInfo though, since that component is rendered larger.

Also a fun idea to consider on this line is grayscale: filter: 'blur(4px) grayscale(1)'. Not sure if we want it though.

I think at this point we can put a placeholder image of a real face here and then see what design thinks.

@@ -136,8 +136,9 @@ const useStyles = makeStyles((theme: Theme) =>
},
switchedOffMessage: {
opacity: 0,
textShadow: '0 0 3px rgba(0, 0, 0, 0.7)',
Copy link
Contributor

Choose a reason for hiding this comment

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

Same, let's add this to MainParticpantInfo too.

Base automatically changed from feature/grid-mode to master July 6, 2022 16:34
@olipyskoty olipyskoty mentioned this pull request Jul 13, 2022
10 tasks
@olipyskoty
Copy link
Contributor Author

Created new PR in #731 to avoid large merge conflict

@olipyskoty olipyskoty closed this Jul 13, 2022
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