Finalize 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-10220

Description

This PR makes some final changes to the grid view behavior:

• Removes white borders (no changes made to dominant speaker border)
• Changes the max grid participants options to 4, 9, 16, 25
• Makes grid view the default view
• Updates naming from "Collaboration Mode" and "Grid Mode" to Collaboration View and Grid View to be consistent with iOS
• Updates screenshare behavior so that users will return to whatever room view they were using prior to screenshare starting
• Changes pagination behavior so that each page has unique participants and none carry over from a previous page

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]

Suggested change

	<Typography variant="body1">{isGridViewActive ? 'Collaboration View' : 'Grid View'}</Typography>
	<Typography variant="body1">{isGridViewActive ? 'Presentation View' : 'Grid View'}</Typography>

[timmydoza]

This is used in a few spots, so it would probably be good to add to the theme.

[timmydoza]

🙂

[timmydoza]

What do you think about adding a border-radius of, say, 6px or 8px? Without the white border in grid view (which I think looks great), the corners feel a touch too sharp. I think we could even have presentation view use the same border-radius of 6 or 8px. Or presentation view could stay at 4px. It'll take some playing with but I think that the grid view corners could be more rounded. What do you think?

[timmydoza]

Also, not sure why I haven't noticed this before, but I think the identity class could use padding of padding: 0.18em 0.3em 0.18em 0;. Without this, the microphone icon doesn't feel exactly centered. What do you think?

[olipyskoty]

Yeah I think that all looks a lot better!

[timmydoza]

Nice comments!

[timmydoza]

Oh, but it should be "presentation view" here.

[timmydoza]

Hmm, we'll need to think about this one. Not sure this needs to be solved in this PR, I but changing this value here doesn't actually change the aspect ratio. I think the fix might be easy though: the container class in ParticipantInfo doesn't need to enforce an aspect ratio in grid view with padding-top. Instead it just needs height: 100%.

I realize this is out of scope for this PR, but it had never occured to me to try to change this constant before 😆

[olipyskoty]

Okay cool, we can look at this later!

[timmydoza]

Here I'm thinking that we might want a black (#000) background for the participants. This is in case we have a participant whose video is a different aspect ratio than its container (like a mobile user).

Here's an example (this isn't what a mobile user would look like but you get the idea):

[timmydoza]

Doesn't make a huge difference, but I'm wondering if this hook should be moved into the GridView directory. Just so people don't think it should be used with MobileGridView pagination.

[olipyskoty]

I think that's a good idea!

[timmydoza]

What do you think about centered here? In some cases, flex-start makes the participants on the last page higher than all the rest, like this:

I think centered participants on the last page might be ideal, but I'm curious what you think? It also has the advantage of getting rid of all the lastPage stuff.

[olipyskoty]

Yep sounds good. I really didn't have an opinion here, and I didn't know what was best. So centering it is!

[timmydoza]

Suggested change

	rerender({ screenShareParticipant: {} } as any);
	rerender({ screenShareParticipant: undefined } as any);

Should correctly end screen share.

[timmydoza]

Suggested change

	it('should not reactivate grid view when screenshare ends if grid view was active before another participant started screensharing', () => {
	it('should not activate grid view when screenshare ends if presentation view was active before another participant started screensharing', () => {

[timmydoza]

Suggested change

	jest.spyOn(React, 'useRef').mockReturnValue({
	current: false,
	});

Same as above.

[timmydoza]

Suggested change

	jest.spyOn(React, 'useRef').mockReturnValue({
	current: true,
	});

I don't think this is needed. Here I think we want to test the real functionality of useRef, and not a mocked return value. But also, this code doesn't do much anyway because the mocked value is overwritten by the first useEffect in useSetPresentaionViewOnScreenShare()

[timmydoza]

I like the comments from the test above. But also, I think we need to rerender the hook once more to end the screensharing:

    expect(mockSetIsGridViewActive).not.toBeCalled();
    // screenshare starts
    rerender({ screenShareParticipant: {} } as any);
    expect(mockSetIsGridViewActive).toBeCalledWith(false);

    // screenshare ends
    rerender({ screenShareParticipant: undefined } as any);

    // mockSetIsGridViewActive should only be called once with "false" since we're not reactivating grid mode
    expect(mockSetIsGridViewActive).toBeCalledTimes(1);

[timmydoza]

Once more test could be nice to test that presentation mode is not activated if the user switches to grid mode during a screenshare:

  it('should not activate presentation view when screenshare ends if presentation view was active before screensharing, but the user switched to grid view during the screenshare', () => {
    const screenShareParticipant = {};
    const room = { localParticipant: {} } as any;

    const { rerender } = renderHook(
      ({ screenShareParticipant, isGridViewActive }) =>
        useSetCollaborationViewOnScreenShare(screenShareParticipant, room, mockSetIsGridViewActive, isGridViewActive),
      { initialProps: { screenShareParticipant: undefined, isGridViewActive: false } }
    );

    expect(mockSetIsGridViewActive).not.toBeCalled();

    // start screenshare
    rerender({ screenShareParticipant, isGridViewActive: false } as any);
    expect(mockSetIsGridViewActive).toBeCalledWith(false);

    // enable grid mode
    rerender({ screenShareParticipant, isGridViewActive: true } as any);

    // stop screenshare
    rerender({ screenShareParticipant: undefined, isGridViewActive: true } as any);

    // mockSetIsGridViewActive should only be called once with "false" since we're not reactivating grid mode
    expect(mockSetIsGridViewActive).toBeCalledTimes(1);
  });

I wouldn't mind a more concise description if you can think of one 😆.

Also, note that I saved the room object and screenshare object to their own variables. This is so hook rerenders happen with the same object references as the previous render. This prevents the useEffects from running unnecessarily. This wasn't needed in the previous tests, but it's needed in this one.

[olipyskoty]

Removed a couple of words but couldn't really come up with anything much shorter 😢

[timmydoza]

Hmm, not sure how I didn't see this before, but it looks like there's a black border around the participants:

I think it's really just a transparent border that's showing the black background. So I guess we just make the border the same color as the grid background color here?

Also I think background: #000000 can be removed here. Looks like there's already background: 'black' in the container class.

[timmydoza]

I think a quick comment here about what this does would be good.

[timmydoza]

Same, comment would be good.