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

VIDCS-3321: Implement participant list toggle button, chat button, report issue button on mobile #95

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import ToolbarButton from '../ToolbarButton';
import PopupDialog, { DialogTexts } from '../PopupDialog';
import { startArchiving, stopArchiving } from '../../../api/archiving';
import useSessionContext from '../../../hooks/useSessionContext';
import useIsSmallViewport from '../../../hooks/useIsSmallViewport';

/**
* ArchivingToggle Component
Expand Down Expand Up @@ -67,6 +68,7 @@ const ArchivingToggle = (): ReactElement => {
handleDialogClick(isRecording ? 'stop' : 'start');
};

const isSmallViewport = useIsSmallViewport();
return (
<>
<Tooltip title={title} aria-label="video layout">
Expand All @@ -78,6 +80,10 @@ const ArchivingToggle = (): ReactElement => {
style={{ color: `${isRecording ? 'rgb(239 68 68)' : 'white'}` }}
/>
}
sx={{
marginTop: isSmallViewport ? '0px' : '4px',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: you'll notice that we set this here instead of ToolbarButton. the reason is that we want to lower only certain buttons to make sure that they align correctly with those that are displayed only on desktop

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we know why these ones are affected only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

these are all the ones that in the dropdown kebab menu but ToolbarButton is used across other components so it's kinda the same reason as here: #95 (comment)

}}
isSmallViewPort={isSmallViewport}
/>
</Tooltip>
<PopupDialog
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { blue } from '@mui/material/colors';
import { Badge } from '@mui/material';
import { ReactElement } from 'react';
import ToolbarButton from '../ToolbarButton';
import useIsSmallViewport from '../../../hooks/useIsSmallViewport';

export type ChatToggleButtonProps = {
handleClick: () => void;
Expand All @@ -27,6 +28,7 @@ const ChatToggleButton = ({
isOpen,
unreadCount,
}: ChatToggleButtonProps): ReactElement => {
const isSmallViewport = useIsSmallViewport();
return (
<Tooltip title={isOpen ? 'Close chat' : 'Open chat'} aria-label="toggle chat">
<Badge
Expand All @@ -42,12 +44,14 @@ const ChatToggleButton = ({
overlap="circular"
>
<ToolbarButton
data-testid="chat-toggle"
sx={{
marginTop: '0px',
marginRight: '0px',
}}
onClick={handleClick}
icon={<ChatIcon sx={{ color: isOpen ? blue.A100 : 'white' }} />}
isSmallViewPort={isSmallViewport}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we not just access the hook value inside the ToolbarButton?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed here: #95 (comment) 🙏

/>
</Badge>
</Tooltip>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { EmojiEmotions } from '@mui/icons-material';
import { Dispatch, ReactElement, SetStateAction, useRef } from 'react';
import ToolbarButton from '../ToolbarButton';
import EmojiGrid from '../EmojiGrid/EmojiGrid';
import useIsSmallViewport from '../../../hooks/useIsSmallViewport';

export type EmojiGridProps = {
isEmojiGridOpen: boolean;
Expand All @@ -29,7 +30,7 @@ const EmojiGridButton = ({
const handleToggle = () => {
setIsEmojiGridOpen((prevOpen) => !prevOpen);
};

const isSmallViewport = useIsSmallViewport();
return (
<>
<Tooltip title="Express yourself" aria-label="open sendable emoji menu">
Expand All @@ -42,6 +43,10 @@ const EmojiGridButton = ({
}
ref={anchorRef}
data-testid="emoji-grid-toggle"
sx={{
marginTop: isSmallViewport ? '0px' : '4px',
}}
isSmallViewPort={isSmallViewport}
/>
</Tooltip>

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import WindowIcon from '@mui/icons-material/Window';
import { ReactElement } from 'react';
import useSessionContext from '../../../hooks/useSessionContext';
import ToolbarButton from '../ToolbarButton';
import useIsSmallViewport from '../../../hooks/useIsSmallViewport';

type LayoutToggleButtonProps = {
isScreenSharePresent: boolean;
Expand Down Expand Up @@ -36,6 +37,8 @@ const LayoutToggleButton = ({
return isGrid ? 'Switch to Active Speaker layout' : 'Switch to Grid layout';
};

const isSmallViewport = useIsSmallViewport();

return (
<Tooltip title={getTooltipTitle()} aria-label="video layout">
<ToolbarButton
Expand All @@ -50,7 +53,9 @@ const LayoutToggleButton = ({
}
sx={{
cursor: isScreenSharePresent ? 'not-allowed' : 'pointer',
marginTop: isSmallViewport ? '0px' : '4px',
}}
isSmallViewPort={isSmallViewport}
/>
</Tooltip>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { blue } from '@mui/material/colors';
import { Badge } from '@mui/material';
import { ReactElement } from 'react';
import ToolbarButton from '../ToolbarButton';
import useIsSmallViewport from '../../../hooks/useIsSmallViewport';

export type ParticipantListButtonProps = {
handleClick: () => void;
Expand All @@ -26,6 +27,7 @@ const ParticipantListToggleButton = ({
isOpen,
participantCount,
}: ParticipantListButtonProps): ReactElement => {
const isSmallViewport = useIsSmallViewport();
return (
<div className="pr-3">
<Tooltip
Expand All @@ -43,12 +45,14 @@ const ParticipantListToggleButton = ({
overlap="circular"
>
<ToolbarButton
data-testid="participant-list-toggle"
sx={{
marginTop: '0px',
marginRight: '0px',
}}
onClick={handleClick}
icon={<PeopleIcon sx={{ color: isOpen ? blue.A100 : 'white' }} />}
isSmallViewPort={isSmallViewport}
/>
</Badge>
</Tooltip>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { Tooltip } from '@mui/material';
import { ReactElement, useRef } from 'react';
import FeedbackIcon from '@mui/icons-material/Feedback';
import ToolbarButton from '../ToolbarButton';
import displayOnDesktop from '../../../utils/displayOnDesktop';
import useIsSmallViewport from '../../../hooks/useIsSmallViewport';

export type ReportIssueButtonProps = {
handleClick: () => void;
Expand All @@ -18,8 +18,9 @@ export type ReportIssueButtonProps = {
*/
const ReportIssueButton = ({ handleClick, isOpen }: ReportIssueButtonProps): ReactElement => {
const anchorRef = useRef<HTMLButtonElement>(null);
const isSmallViewport = useIsSmallViewport();
return (
<div className={`hidden ${displayOnDesktop()} px-3`}>
<div className="pr-3">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm this is different from before px-3, will this change anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this doesn't seem to impact anything on desktop:

image

this is with px-3 in comparison on desktop:

image

but on mobile, switching to pr-3 helps with the gaps:

image

this is with px-3 in comparison:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you remove the right margin for the chat button? I think it's interfering with the flex layout. There's more padding on the right side versus the left.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ChatButton did not have any right margin but I added a little bit on the layout toggle so that it aligns a bit better.

iPhone 14 Pro Max:

image image

<Tooltip title="Report issue" aria-label="open report issue menu">
<ToolbarButton
data-testid="report-issue-button"
Expand All @@ -30,6 +31,7 @@ const ReportIssueButton = ({ handleClick, isOpen }: ReportIssueButtonProps): Rea
onClick={handleClick}
icon={<FeedbackIcon style={{ color: `${!isOpen ? 'white' : 'rgb(138, 180, 248)'}` }} />}
ref={anchorRef}
isSmallViewPort={isSmallViewport}
/>
</Tooltip>
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ export type ToolbarButtonProps = {
icon: ReactElement;
sx?: SxProps;
id?: string;
isSmallViewPort?: boolean;
};

/**
Expand All @@ -16,7 +17,9 @@ export type ToolbarButtonProps = {
* @param {ToolbarButtonProps} props - props for the component
* @property {Function} onClick - on click handler
* @property {ReactElement} icon - MUI Icon for button
* @property {SxProps} sx- optional MUI style object
* @property {SxProps} sx - (optional) MUI style object
* @property {string} id - (optional) the data-testid used in unit tests
* @property {boolean} isSmallViewPort - (optional) indicates whether the device is a small view port.
* @returns {ReactElement}
*/
const ToolbarButton = forwardRef(function ToolbarButton(
Expand All @@ -35,8 +38,8 @@ const ToolbarButton = forwardRef(function ToolbarButton(
marginLeft: '0px',
marginTop: '4px',
marginRight: '12px',
width: '48px',
height: '48px',
width: props.isSmallViewPort ? '42px' : '48px',
height: props.isSmallViewPort ? '42px' : '48px',
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we just use the hook here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so it is a little tricky because if we use the hook here it'll apply it to all of the toolbar buttons and we don't want that. The reason being the kebab menu and the hand up menus are ToolbarButton components while the other ones (audio and video dropdowns) are not:
image

this way it'll get only applied to certain buttons

backgroundColor: 'rgba(60, 64, 67, 0.55)',
'&:hover': {
backgroundColor: 'rgba(60, 64, 67, 0.42)',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,26 @@ describe('ToolbarOverflowMenu', () => {

expect(screen.queryByTestId('toolbar-overflow-menu')).not.toBeInTheDocument();
});

it('renders all the available buttons including the Report Issue button if enabled', () => {
vi.stubEnv('VITE_ENABLE_REPORT_ISSUE', 'true');
render(<TestComponent defaultOpen />);
expect(screen.getByTestId('layout-toggle')).toBeVisible();
expect(screen.getByTestId('archiving-toggle')).toBeVisible();
expect(screen.getByTestId('emoji-grid-toggle')).toBeVisible();
expect(screen.getByTestId('report-issue-button')).toBeVisible();
expect(screen.getByTestId('participant-list-toggle')).toBeVisible();
expect(screen.getByTestId('chat-toggle')).toBeVisible();
});

it('does not render Report Issue button in overflow menu if it was disabled', () => {
vi.stubEnv('VITE_ENABLE_REPORT_ISSUE', 'false');
render(<TestComponent defaultOpen />);
expect(screen.getByTestId('layout-toggle')).toBeVisible();
expect(screen.getByTestId('archiving-toggle')).toBeVisible();
expect(screen.getByTestId('emoji-grid-toggle')).toBeVisible();
expect(screen.queryByTestId('report-issue-button')).not.toBeInTheDocument();
expect(screen.getByTestId('participant-list-toggle')).toBeVisible();
expect(screen.getByTestId('chat-toggle')).toBeVisible();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ import { Dispatch, ReactElement, RefObject, SetStateAction } from 'react';
import ArchivingToggle from '../ArchivingToggle';
import EmojiGridButton from '../EmojiGridButton';
import LayoutToggleButton from '../LayoutToggleButton';
import ParticipantListToggleButton from '../ParticipantListToggleButton';
import ChatToggleButton from '../ChatToggleButton';
import ReportIssueButton from '../ReportIssueButton';
import useSessionContext from '../../../hooks/useSessionContext';

export type ToolbarOverflowMenuProps = {
Expand Down Expand Up @@ -33,9 +36,18 @@ const ToolbarOverflowMenu = ({
anchorRef,
handleClickAway,
}: ToolbarOverflowMenuProps): ReactElement => {
const { subscriberWrappers } = useSessionContext();
const {
subscriberWrappers,
rightPanelActiveTab,
toggleParticipantList,
toggleChat,
toggleReportIssue,
unreadCount,
} = useSessionContext();
const isViewingScreenShare = subscriberWrappers.some((subWrapper) => subWrapper.isScreenshare);

const participantCount =
subscriberWrappers.filter(({ isScreenshare }) => !isScreenshare).length + 1;
const isReportIssueEnabled = import.meta.env.VITE_ENABLE_REPORT_ISSUE === 'true';
return (
<Popper
open={isToolbarOverflowMenuOpen}
Expand Down Expand Up @@ -82,6 +94,22 @@ const ToolbarOverflowMenu = ({
isParentOpen={isToolbarOverflowMenuOpen}
/>
<ArchivingToggle />
{isReportIssueEnabled && (
<ReportIssueButton
isOpen={rightPanelActiveTab === 'issues'}
handleClick={toggleReportIssue}
/>
)}
<ParticipantListToggleButton
isOpen={rightPanelActiveTab === 'participant-list'}
handleClick={toggleParticipantList}
participantCount={participantCount}
/>
<ChatToggleButton
isOpen={rightPanelActiveTab === 'chat'}
handleClick={toggleChat}
unreadCount={unreadCount}
/>
</Paper>
</ClickAwayListener>
</div>
Expand Down