-
Notifications
You must be signed in to change notification settings - Fork 4
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
base: develop
Are you sure you want to change the base?
VIDCS-3321: Implement participant list toggle button, chat button, report issue button on mobile #95
Changes from 7 commits
51ccfe9
817f02b
e161457
d1905e1
e0962ac
1fe7848
5ce1373
f967716
7e13973
3bcdebf
9bba753
662c16c
fc25f04
db56dc7
afd5b01
e0314a2
f62f6e2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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 | ||
|
@@ -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} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we not just access the hook value inside the ToolbarButton? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. addressed here: #95 (comment) 🙏 |
||
/> | ||
</Badge> | ||
</Tooltip> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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`}> | ||
behei-vonage marked this conversation as resolved.
Show resolved
Hide resolved
|
||
<div className="pr-3"> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm this is different from before There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
<Tooltip title="Report issue" aria-label="open report issue menu"> | ||
<ToolbarButton | ||
data-testid="report-issue-button" | ||
|
@@ -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> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ export type ToolbarButtonProps = { | |
icon: ReactElement; | ||
sx?: SxProps; | ||
id?: string; | ||
isSmallViewPort?: boolean; | ||
}; | ||
|
||
/** | ||
|
@@ -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( | ||
|
@@ -35,8 +38,8 @@ const ToolbarButton = forwardRef(function ToolbarButton( | |
marginLeft: '0px', | ||
behei-vonage marked this conversation as resolved.
Show resolved
Hide resolved
|
||
marginTop: '4px', | ||
marginRight: '12px', | ||
width: '48px', | ||
height: '48px', | ||
width: props.isSmallViewPort ? '42px' : '48px', | ||
height: props.isSmallViewPort ? '42px' : '48px', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we just use the hook here? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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)', | ||
|
There was a problem hiding this comment.
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 desktopThere was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)