Video 8473 mobile grid mode

[olipyskoty]

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):

• VIDEO-8473

Description

This PR adds support for grid view on mobile.

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]

Here I think we need to specify something so that we can see the inactive pagination dots. Digging through devtools, it looks like we can update the --swiper-pagination-bullet-inactive-color CSS variable to be white instead of black. I think adding this here seems to do the trick:

'--swiper-pagination-bullet-inactive-color': 'white',

Then we can see the total number of pages:

[timmydoza]

Suggested change

	height: '90%',
	height: '90%', // To leave room for the pagination indicators

[timmydoza]

All the values in this function stood out to me. It looks strange that they aren't more even fractions, like one half (50%), or one third (33.33%). Looks like a little bit was taken off to account for the 0.2em of margin. This pattern seems funny to me, because it's reducing the size of a participant by an amount that's measured by a percentage of its parent container (here it's 2.5% for the height, and 1.33% for the width), to account for a margin that's measured as a fraction of the closest parent element's font-size. The units aren't the same (% vs. em), plus it's not clear how the percentage values were calculated (height is 48% in one spot and 47.5% in another). Here I think it's best to have exact math if we can (especially since a very small change in percentage can result in a visible discrepancy, so precision matters here. For instance, for a container that's 788px tall, there's going to be a difference of about 2.4px depending on whether or not we go with a height of 33.3% or 33.33%. Multiply this difference by three participants and we can see that the stack of three participants is about 7.2px shorter than a single participant with height of 100%).

So I went ahead and changed the values to more exact percentages (100%, 50%, and 33.33%). This broke the layout because the margin makes the participants too large, but it turns out that there's an easy way to accommodate for this:

Instead of margin: 0.2em, we can use padding: 0.2em, which looks just as bad, but with padding, we can tell the browser to take padding into account when we specify a container size. This is done with box-sizing: 'border-box'. More info on what that does here: https://developer.mozilla.org/en-US/docs/Web/CSS/box-sizing

So with a new CSS class that has padding: 0.2em and box-sizing: border-box, and the precise percentage values, this layout seems to work out just perfect (literally 🙂 ).

[timmydoza]

I like the switch statement. It's very readable, but I couldn't help but think of how it could be refactored.

Since the cases in the switch statement are essentially looking at a numerical value, I got the idea that we could retrieve our percentage values from a map:

  {
    0: '100%',
    1: '50%',
    2: // so on...
  }

And of course, an object with numbers as keys is just an array (and we can use Math.min so the indicies beyond 4 are never accessed):

const height = ['100%', '50%', '33.33%', '50%', '33.33%'][Math.min(remoteParticipantCount, 4)]

For width, there are only two values, which could just be a ternary:

const width = remoteParticipantCount < 3 ? '100%' : '50%'

With all this, a neat refactor could look like this:

              <div
                style={{
                  width: remoteParticipantCount < 3 ? '100%' : '50%',
                  height: ['100%', '50%', '33.33%', '50%', '33.33%'][Math.min(remoteParticipantCount, 4)],
                }}
                key={i}
                className={classes.participantContainer}
              >
                <Participant participant={participant} isLocalParticipant={room!.localParticipant === participant} />
              </div>

Not sure if it's more or less readable, but it's more concise. Let me know what you think!

[olipyskoty]

I like it! I do think that the new height property needs a comment though. Took me a second to grasp how it is working. So to save other developers a few seconds of their time, I extracted the styles into its own object with a comment.

[timmydoza]

participants.length is accessed a few times in this file. It also isn't exactly clear if participants is dealing with remote participants or all participants. An easy fix for readability would be to just save this value to a variable to give it a nice name:

const remoteParticipantCount = participants.length

[timmydoza]

Not sure if alignContent is needed. Is it doing anything?

[olipyskoty]

Doesn't look like it. It was at one point when I was messing around with the CSS, but looks like it is useless now 🙂

[timmydoza]

I'm curious what this one looks like with real people in the room. I think this is what iOS does, but I think it's worth playing around with. It's easy enough to change 🙂

[olipyskoty]

From what I saw, it looks pretty good when there are 1-2 participants and 4 or greater participants. When there are three participants it looks a little odd due to being 100% wide and only 33% tall. It's as if you only get to see a part of the participant's video. Seeing what others think will be helpful though!

[timmydoza]

So I came across this fix accidentally, but it turns out that this change here isn't needed. When box-sizing: border-box is added in MobileGridView, it gets applied to this component as well, which completely changes the size of the element. I think it's because the default style for box-sizing on a div is inherit.

[timmydoza]

This is clearly needed to make the tests pass, but I can't understand why 😭 . Everything is shallow rendered. Do you have any ideas?

[olipyskoty]

Jest is failing due to a syntax error: SyntaxError: Cannot use import statement outside a module. Sent you the screenshot in slack.. not entirely sure why mocking the file makes it pass though 🤔

[timmydoza]

Are there package-lock changes to commit?

[timrozum]

The demo looks great!