Video 10105 finalize audience chat #136
Conversation
apps/web/src/App.tsx
Outdated
| @@ -50,7 +52,7 @@ export default function App() { | |||
| <RecordingNotifications /> | |||
| <MobileTopMenuBar /> | |||
| <Room /> | |||
| <MenuBar /> | |||
| {isMobile ? <MobileBottomMenuBar /> : <MenuBar />} | |||
There was a problem hiding this comment.
I think we should try to just use <MenuBar /> only, but if we had <MobileBottomMenuBar /> too then we should use this to switch between the two:
const isMobile = useMediaQuery((theme: Theme) => theme.breakpoints.down('sm'));
isMobile from utils just looks at the user agent. For styling on mobile, we should use media queries.
| @@ -54,7 +54,7 @@ export default function LeaveEventButton(props: { buttonClassName?: string }) { | |||
| return ( | |||
| <> | |||
| <Button onClick={() => setMenuOpen(isOpen => !isOpen)} ref={anchorRef} className={classes.button}> | |||
| Leave Event | |||
| {!props.excludeLabel && 'Leave Event'} | |||
There was a problem hiding this comment.
Can this work instead in all buttons?
<Hidden smDown>
Leave Event
</Hidden>
| const isReconnecting = roomState === 'reconnecting'; | ||
| const { appState } = useAppState(); | ||
|
|
||
| return ( |
There was a problem hiding this comment.
Much of this seems very similar to the MenuBar component. I wonder if we truly need a new MobileBottomMenuBar component. Can we use <Hidden /> in MenuBar to get it to work on mobile?
| @@ -1,6 +1,7 @@ | |||
| import React, { useCallback, useRef, useState } from 'react'; | |||
| import { createStyles, makeStyles, Theme } from '@material-ui/core/styles'; | |||
| import { Typography, Grid, Button } from '@material-ui/core'; | |||
| import { isMobile } from '../../../utils'; | |||
There was a problem hiding this comment.
Will <Hidden> work instead?
Also we should not use this isMobile to adjust styles. Using the userAgent to adjust styles on mobile is not responsive - the UI cannot adjust itself in response to changes in browser width.
| display: 'none', | ||
| }, | ||
| '& .MuiButton-mobileBackground': { | ||
| marginRight: '1em', |
There was a problem hiding this comment.
| marginRight: '1em', | |
| margin: '0 0.5em', |
This will cause the buttons to be perfectly centered. With marginRight, it's a little lopsided. There's a little more white space on the right than on the left:
Contributing to Twilio
Pull Request Details
JIRA link(s):
Description
This PR has the chat window open by default for desktop. It also fixes a bug where the chat button was not being displayed for viewers even when Chat was enabled 馃槦. Finally, it cleans up the bottom menu bar for mobile (for all types of participants)
Speaker
Viewer
Burndown
Before review
Before merge