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.

Sign up for GitHub

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

Jump to bottom

Finalize grid mode #699

Merged
merged 17 commits into from
Jun 10, 2022
Merged

Finalize grid mode #699

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
1442950
change name from grid mode to grid view
olipyskoty Jun 7, 2022
35ccd13
update pagination participants to be consistent with iOS
olipyskoty Jun 7, 2022
a1d4c6e
update max grid participants options
olipyskoty Jun 8, 2022
920ff1e
fix failing GridView test
olipyskoty Jun 8, 2022
4c5490e
fix failing usePagination tests
olipyskoty Jun 8, 2022
ab05ffb
update screensharing behavior
olipyskoty Jun 8, 2022
fe2738d
fix failing room tests
olipyskoty Jun 8, 2022
2690dc8
have cypress tests use collab view
olipyskoty Jun 9, 2022
0d40256
add gridview background to theme and move usePagination to /GridView
olipyskoty Jun 9, 2022
c29ad7d
add padding to participant tiles
olipyskoty Jun 9, 2022
0b36cbc
change "Collaboration" to "Presentation"
olipyskoty Jun 9, 2022
10dcd10
deactivate grid view for cypress tests
olipyskoty Jun 9, 2022
b1a0e71
update room tests
olipyskoty Jun 9, 2022
a365f34
remove unneeded beforeEach hooks in cypress tests
olipyskoty Jun 9, 2022
e5f8b1f
fix eslint error
olipyskoty Jun 9, 2022
55f7c9f
change participant border color to match grid background
olipyskoty Jun 9, 2022
4e3fbaa
add comments to cypress tests for clarity
olipyskoty Jun 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/components/DeviceSelectionDialog/MaxGridParticipants/MaxGridParticipants.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import React from 'react';
import { FormControl, MenuItem, Typography, Select, Grid } from '@material-ui/core';
import { useAppState } from '../../../state';

const MAX_PARTICIPANT_OPTIONS = [9, 16, 25, 36, 49];
const MAX_PARTICIPANT_OPTIONS = [4, 9, 16, 25];

export default function MaxGridParticipants() {
const { maxGridParticipants, setMaxGridParticipants } = useAppState();
Expand Down
6 changes: 3 additions & 3 deletions src/components/GridView/GridView.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ const mockParticipants = [
];

jest.mock('../../constants', () => ({
GRID_MODE_ASPECT_RATIO: 9 / 16,
GRID_MODE_MARGIN: 3,
GRID_VIEW_ASPECT_RATIO: 9 / 16,
GRID_VIEW_MARGIN: 3,
}));
jest.mock('../../hooks/useCollaborationParticipants/useCollaborationParticipants', () => () => mockParticipants);
jest.mock('../../hooks/useVideoContext/useVideoContext', () => () => ({
Expand Down Expand Up @@ -50,7 +50,7 @@ describe('the GridView component', () => {
it('should render correctly', () => {
const wrapper = shallow(<GridView />);
expect(wrapper).toMatchSnapshot();
expect(useGridLayout).toHaveBeenCalledWith(5);
expect(useGridLayout).toHaveBeenCalledWith(9);
});

it('should not render the previous page button when the user is viewing the first page', () => {
Expand Down
22 changes: 16 additions & 6 deletions src/components/GridView/GridView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import React from 'react';
import ArrowBack from '@material-ui/icons/ArrowBack';
import ArrowForward from '@material-ui/icons/ArrowForward';
import clsx from 'clsx';
import { GRID_MODE_ASPECT_RATIO, GRID_MODE_MARGIN } from '../../constants';
import { GRID_VIEW_ASPECT_RATIO, GRID_VIEW_MARGIN } from '../../constants';
import { IconButton, makeStyles } from '@material-ui/core';
import { Pagination } from '@material-ui/lab';
import Participant from '../Participant/Participant';
Expand All @@ -11,11 +11,13 @@ import useVideoContext from '../../hooks/useVideoContext/useVideoContext';
import { usePagination } from '../../hooks/usePagination/usePagination';
import useDominantSpeaker from '../../hooks/useDominantSpeaker/useDominantSpeaker';
import useGridParticipants from '../../hooks/useGridParticipants/useGridParticipants';
import { useAppState } from '../../state';

const CONTAINER_GUTTER = '50px';

const useStyles = makeStyles({
container: {
background: '#121C2D',
Copy link
Contributor

Choose a reason for hiding this comment

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

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

position: 'relative',
gridArea: '1 / 1 / 2 / 3',
},
Expand Down Expand Up @@ -72,23 +74,28 @@ const useStyles = makeStyles({
alignItems: 'center',
justifyContent: 'center',
},
lastPage: {
alignContent: 'flex-start',
Copy link
Contributor

Choose a reason for hiding this comment

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

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:

centered

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

},
});

export function GridView() {
const classes = useStyles();
const { maxGridParticipants } = useAppState();
const { room } = useVideoContext();
const gridParticipants = useGridParticipants();
const dominantSpeaker = useDominantSpeaker(true);

const { paginatedParticipants, setCurrentPage, currentPage, totalPages } = usePagination([
const { paginatedParticipants, setCurrentPage, currentPage, totalPages, isLastPage } = usePagination([
room!.localParticipant,
...gridParticipants,
]);

const { participantVideoWidth, containerRef } = useGridLayout(paginatedParticipants.length);
const gridLayoutParticipantCount = currentPage === 1 ? paginatedParticipants.length : maxGridParticipants;
const { participantVideoWidth, containerRef } = useGridLayout(gridLayoutParticipantCount);

const participantWidth = `${participantVideoWidth}px`;
const participantHeight = `${Math.floor(participantVideoWidth * GRID_MODE_ASPECT_RATIO)}px`;
const participantHeight = `${Math.floor(participantVideoWidth * GRID_VIEW_ASPECT_RATIO)}px`;

return (
<div className={classes.container}>
Expand Down Expand Up @@ -121,11 +128,14 @@ export function GridView() {
/>
)}
</div>
<div className={classes.participantContainer} ref={containerRef}>
<div
className={clsx(classes.participantContainer, { [classes.lastPage]: currentPage > 1 && isLastPage })}
ref={containerRef}
>
{paginatedParticipants.map(participant => (
<div
key={participant.sid}
style={{ width: participantWidth, height: participantHeight, margin: GRID_MODE_MARGIN }}
style={{ width: participantWidth, height: participantHeight, margin: GRID_VIEW_MARGIN }}
>
<Participant
participant={participant}
Expand Down
34 changes: 17 additions & 17 deletions src/components/MenuBar/Menu/Menu.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ mockUseLocalVideoToggle.mockImplementation(() => [true, () => {}]);

describe('the Menu component', () => {
let mockUpdateRecordingRules: jest.Mock<any>;
let mockSetIsGridModeActive = jest.fn();
let mockSetIsGridViewActive = jest.fn();

beforeEach(() => jest.clearAllMocks());

Expand All @@ -48,7 +48,7 @@ describe('the Menu component', () => {
isFetching: false,
updateRecordingRules: mockUpdateRecordingRules,
roomType: 'group',
setIsGridModeActive: mockSetIsGridModeActive,
setIsGridViewActive: mockSetIsGridViewActive,
}));
mockUseFlipCameraToggle.mockImplementation(() => ({
flipCameraDisabled: false,
Expand Down Expand Up @@ -90,7 +90,7 @@ describe('the Menu component', () => {
isFetching: false,
updateRecordingRules: mockUpdateRecordingRules,
roomType: 'group',
setIsGridModeActive: mockSetIsGridModeActive,
setIsGridViewActive: mockSetIsGridViewActive,
}));
const { getByText } = render(<Menu />);
fireEvent.click(getByText('More'));
Expand All @@ -102,7 +102,7 @@ describe('the Menu component', () => {
isFetching: false,
updateRecordingRules: mockUpdateRecordingRules,
roomType: 'group-small',
setIsGridModeActive: mockSetIsGridModeActive,
setIsGridViewActive: mockSetIsGridViewActive,
}));
const { getByText } = render(<Menu />);
fireEvent.click(getByText('More'));
Expand All @@ -114,7 +114,7 @@ describe('the Menu component', () => {
isFetching: false,
updateRecordingRules: mockUpdateRecordingRules,
roomType: 'go',
setIsGridModeActive: mockSetIsGridModeActive,
setIsGridViewActive: mockSetIsGridViewActive,
}));
const { getByText, queryByText } = render(<Menu />);
fireEvent.click(getByText('More'));
Expand All @@ -126,7 +126,7 @@ describe('the Menu component', () => {
isFetching: false,
updateRecordingRules: mockUpdateRecordingRules,
roomType: 'peer-to-peer',
setIsGridModeActive: mockSetIsGridModeActive,
setIsGridViewActive: mockSetIsGridViewActive,
}));
const { getByText, queryByText } = render(<Menu />);
fireEvent.click(getByText('More'));
Expand All @@ -138,7 +138,7 @@ describe('the Menu component', () => {
isFetching: false,
updateRecordingRules: mockUpdateRecordingRules,
roomType: undefined,
setIsGridModeActive: mockSetIsGridModeActive,
setIsGridViewActive: mockSetIsGridViewActive,
}));
const { getByText } = render(<Menu />);
fireEvent.click(getByText('More'));
Expand Down Expand Up @@ -210,30 +210,30 @@ describe('the Menu component', () => {
expect(wrapper.find(DeviceSelectionDialog).prop('open')).toBe(true);
});

it('should show the Grid Mode button when grid mode is inactive', () => {
it('should show the Grid View button when grid view is inactive', () => {
mockUseAppState.mockImplementation(() => ({
setIsGridModeActive: mockSetIsGridModeActive,
isGridModeActive: false,
setIsGridViewActive: mockSetIsGridViewActive,
isGridViewActive: false,
}));

const { getByText } = render(<Menu />);
fireEvent.click(getByText('More'));
fireEvent.click(getByText('Grid Mode'));
fireEvent.click(getByText('Grid View'));

expect(mockSetIsGridModeActive.mock.calls[0][0](false)).toBe(true);
expect(mockSetIsGridViewActive.mock.calls[0][0](false)).toBe(true);
});

it('should show the Collaboration Mode button when grid mode is active', () => {
it('should show the Collaboration View button when grid view is active', () => {
mockUseAppState.mockImplementation(() => ({
setIsGridModeActive: mockSetIsGridModeActive,
isGridModeActive: true,
setIsGridViewActive: mockSetIsGridViewActive,
isGridViewActive: true,
}));

const { getByText } = render(<Menu />);
fireEvent.click(getByText('More'));
fireEvent.click(getByText('Collaboration Mode'));
fireEvent.click(getByText('Collaboration View'));

expect(mockSetIsGridModeActive.mock.calls[0][0](true)).toBe(false);
expect(mockSetIsGridViewActive.mock.calls[0][0](true)).toBe(false);
});

it('should render the correct icon', () => {
Expand Down
8 changes: 4 additions & 4 deletions src/components/MenuBar/Menu/Menu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export default function Menu(props: { buttonClassName?: string }) {
const [menuOpen, setMenuOpen] = useState(false);
const [settingsOpen, setSettingsOpen] = useState(false);

const { isFetching, updateRecordingRules, roomType, setIsGridModeActive, isGridModeActive } = useAppState();
const { isFetching, updateRecordingRules, roomType, setIsGridViewActive, isGridViewActive } = useAppState();
const { setIsChatWindowOpen } = useChatContext();
const isRecording = useIsRecording();
const { room, setIsBackgroundSelectionOpen } = useVideoContext();
Expand Down Expand Up @@ -137,18 +137,18 @@ export default function Menu(props: { buttonClassName?: string }) {

<MenuItem
onClick={() => {
setIsGridModeActive(isGrid => !isGrid);
setIsGridViewActive(isGrid => !isGrid);
setMenuOpen(false);
}}
>
<IconContainer>
{isGridModeActive ? (
{isGridViewActive ? (
<CollaborationViewIcon style={{ fill: '#707578', width: '0.9em' }} />
) : (
<GridViewIcon style={{ fill: '#707578', width: '0.9em' }} />
)}
</IconContainer>
<Typography variant="body1">{isGridModeActive ? 'Collaboration Mode' : 'Grid Mode'}</Typography>
<Typography variant="body1">{isGridViewActive ? 'Collaboration View' : 'Grid View'}</Typography>
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
<Typography variant="body1">{isGridViewActive ? 'Collaboration View' : 'Grid View'}</Typography>
<Typography variant="body1">{isGridViewActive ? 'Presentation View' : 'Grid View'}</Typography>

</MenuItem>

<MenuItem onClick={() => setAboutOpen(true)}>
Expand Down
1 change: 1 addition & 0 deletions src/components/MobileGridView/MobileGridView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { Participant as IParticipant } from 'twilio-video';

const useStyles = makeStyles({
participantContainer: {
background: '#121C2D',
position: 'absolute',
top: 0,
right: 0,
Expand Down
2 changes: 1 addition & 1 deletion src/components/ParticipantInfo/ParticipantInfo.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ const mockUsePublications = usePublications as jest.Mock<any>;
const mockUseIsTrackSwitchedOff = useIsTrackSwitchedOff as jest.Mock<any>;
const mockUseParticipantIsReconnecting = useParticipantIsReconnecting as jest.Mock<boolean>;

mockUseAppState.mockImplementation(() => ({ isGridModeActive: false }));
mockUseAppState.mockImplementation(() => ({ isGridViewActive: false }));

describe('the ParticipantInfo component', () => {
it('should render the AvatarIcon component when no video tracks are published', () => {
Expand Down
24 changes: 13 additions & 11 deletions src/components/ParticipantInfo/ParticipantInfo.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import useIsTrackSwitchedOff from '../../hooks/useIsTrackSwitchedOff/useIsTrackS
import usePublications from '../../hooks/usePublications/usePublications';
import useTrack from '../../hooks/useTrack/useTrack';
import useParticipantIsReconnecting from '../../hooks/useParticipantIsReconnecting/useParticipantIsReconnecting';
import { GRID_MODE_MARGIN } from '../../constants';
import { GRID_VIEW_MARGIN } from '../../constants';
import { useAppState } from '../../state';

const borderWidth = 2;
Expand Down Expand Up @@ -116,7 +116,7 @@ const useStyles = makeStyles((theme: Theme) =>
bottom: 0,
left: 0,
},
typeography: {
typography: {
Copy link
Contributor

Choose a reason for hiding this comment

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

🙂

color: 'white',
[theme.breakpoints.down('sm')]: {
fontSize: '0.75rem',
Expand All @@ -128,11 +128,9 @@ const useStyles = makeStyles((theme: Theme) =>
cursorPointer: {
cursor: 'pointer',
},
dominantSpeaker: {
border: `solid ${borderWidth}px #7BEAA5`,
margin: `${GRID_MODE_MARGIN} - borderWidth`,
},
mobileGridMode: {
gridView: {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think that all looks a lot better!

background: '#121C2D',
Copy link
Contributor

Choose a reason for hiding this comment

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

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

border: `${theme.participantBorderWidth}px solid transparent`,
[theme.breakpoints.down('sm')]: {
position: 'relative',
width: '100%',
Expand All @@ -145,6 +143,10 @@ const useStyles = makeStyles((theme: Theme) =>
},
},
},
dominantSpeaker: {
border: `solid ${borderWidth}px #7BEAA5`,
margin: `${GRID_VIEW_MARGIN} - borderWidth`,
},
})
);

Expand Down Expand Up @@ -181,7 +183,7 @@ export default function ParticipantInfo({
const audioTrack = useTrack(audioPublication) as LocalAudioTrack | RemoteAudioTrack | undefined;
const isParticipantReconnecting = useParticipantIsReconnecting(participant);

const { isGridModeActive } = useAppState();
const { isGridViewActive } = useAppState();

const classes = useStyles();

Expand All @@ -191,7 +193,7 @@ export default function ParticipantInfo({
[classes.hideParticipant]: hideParticipant,
[classes.cursorPointer]: Boolean(onClick),
[classes.dominantSpeaker]: isDominantSpeaker,
[classes.mobileGridMode]: isGridModeActive,
[classes.gridView]: isGridViewActive,
})}
onClick={onClick}
data-cy-participant={participant.identity}
Expand All @@ -206,7 +208,7 @@ export default function ParticipantInfo({
)}
<span className={classes.identity}>
<AudioLevelIndicator audioTrack={audioTrack} />
<Typography variant="body1" className={classes.typeography} component="span">
<Typography variant="body1" className={classes.typography} component="span">
{participant.identity}
{isLocalParticipant && ' (You)'}
</Typography>
Expand All @@ -222,7 +224,7 @@ export default function ParticipantInfo({
)}
{isParticipantReconnecting && (
<div className={classes.reconnectingContainer}>
<Typography variant="body1" className={classes.typeography}>
<Typography variant="body1" className={classes.typography}>
Reconnecting...
</Typography>
</div>
Expand Down