From a31d17607456c7f77e275502761ada62d35a7aba Mon Sep 17 00:00:00 2001 From: Vlad Piersec Date: Mon, 1 Nov 2021 11:39:19 +0200 Subject: [PATCH] fix(Drawer): Close drawer on item click Clicking on an item when the popup drawer is displayed would keep it open. Now clicking on any item should automatically close the drawer. Popup was also refactored and no longer uses refs. --- css/_connection-info.scss | 6 - css/_popover.scss | 4 + css/_popup_menu.scss | 6 + .../base/popover/components/Popover.web.js | 77 +++++++------ .../components/web/ConnectionIndicator.js | 55 ++++++++-- .../filmstrip/components/web/Thumbnail.js | 83 ++++++++------ .../web/MeetingParticipantContextMenu.js | 7 +- .../video-layout/components/TileViewButton.js | 9 +- .../components/web/FlipLocalVideoButton.js | 8 +- .../web/LocalVideoMenuTriggerButton.js | 69 +++++------- .../web/RemoteVideoMenuTriggerButton.js | 103 +++++++++--------- .../components/web/VideoMenuButton.js | 3 +- 12 files changed, 247 insertions(+), 183 deletions(-) diff --git a/css/_connection-info.scss b/css/_connection-info.scss index 95aa1e2fc..b22549276 100644 --- a/css/_connection-info.scss +++ b/css/_connection-info.scss @@ -11,12 +11,6 @@ { @extend %connection-info; - /** - * Apply negative margin to reduce the appearance of padding in AtlasKit - * InlineDialog. - */ - margin: -15px; - > table { white-space: nowrap; @extend %connection-info; diff --git a/css/_popover.scss b/css/_popover.scss index 8c0309522..6e82fda94 100644 --- a/css/_popover.scss +++ b/css/_popover.scss @@ -46,3 +46,7 @@ padding: 16px 24px; z-index: $popoverZ; } + +.padded-content { + padding: 4px 8px; +} diff --git a/css/_popup_menu.scss b/css/_popup_menu.scss index 25a3c5d89..5a2a7fdbc 100644 --- a/css/_popup_menu.scss +++ b/css/_popup_menu.scss @@ -5,6 +5,7 @@ .popupmenu { background-color: $menuBG; border-radius: 3px; + list-style-type: none; min-width: 150px; text-align: left; padding: 0px; @@ -38,6 +39,11 @@ } } + &__list { + margin: 0; + padding: 0; + } + &__text { display: inline-block; margin-left: 8px; diff --git a/react/features/base/popover/components/Popover.web.js b/react/features/base/popover/components/Popover.web.js index 12593405b..2e11f59da 100644 --- a/react/features/base/popover/components/Popover.web.js +++ b/react/features/base/popover/components/Popover.web.js @@ -1,9 +1,10 @@ /* @flow */ - +import clsx from 'clsx'; import React, { Component } from 'react'; import { Drawer, JitsiPortal, DialogPortal } from '../../../toolbox/components/web'; import { isMobileBrowser } from '../../environment/utils'; +import { connect } from '../../redux'; import { getContextMenuStyle } from '../functions.web'; /** @@ -57,7 +58,17 @@ type Props = { * From which side of the dialog trigger the dialog should display. The * value will be passed to {@code InlineDialog}. */ - position: string + position: string, + + /** + * Whether the content show have some padding. + */ + paddedContent: ?boolean, + + /** + * Whether the popover is visible or not. + */ + visible: boolean }; /** @@ -68,12 +79,7 @@ type State = { /** * The style to apply to the context menu in order to position it correctly. */ - contextMenuStyle: Object, - - /** - * Whether or not the {@code InlineDialog} should be displayed. - */ - showDialog: boolean + contextMenuStyle: Object }; /** @@ -110,7 +116,6 @@ class Popover extends Component { super(props); this.state = { - showDialog: false, contextMenuStyle: null }; @@ -127,16 +132,6 @@ class Popover extends Component { this._getCustomDialogStyle = this._getCustomDialogStyle.bind(this); } - /** - * Public method for triggering showing the context menu dialog. - * - * @returns {void} - * @public - */ - showDialog() { - this._onShowDialog(); - } - /** * Sets up a touch event listener to attach. * @@ -164,7 +159,7 @@ class Popover extends Component { * @returns {ReactElement} */ render() { - const { children, className, content, id, overflowDrawer } = this.props; + const { children, className, content, id, overflowDrawer, visible } = this.props; if (overflowDrawer) { return ( @@ -175,7 +170,7 @@ class Popover extends Component { { children } { content } @@ -193,7 +188,7 @@ class Popover extends Component { onMouseEnter = { this._onShowDialog } onMouseLeave = { this._onHideDialog } ref = { this._containerRef }> - { this.state.showDialog && ( + { visible && ( { * @returns {void} */ _onTouchStart(event) { - if (this.state.showDialog + if (this.props.visible && !this.props.overflowDrawer && this._contextMenuRef && this._contextMenuRef.contains @@ -263,7 +258,6 @@ class Popover extends Component { */ _onHideDialog() { this.setState({ - showDialog: false, contextMenuStyle: null }); @@ -284,12 +278,9 @@ class Popover extends Component { */ _onShowDialog(event) { event && event.stopPropagation(); - if (!this.props.disablePopover) { - this.setState({ showDialog: true }); - if (this.props.onPopoverOpen) { - this.props.onPopoverOpen(); - } + if (!this.props.disablePopover) { + this.props.onPopoverOpen(); } } @@ -319,7 +310,7 @@ class Popover extends Component { _onKeyPress(e) { if (e.key === ' ' || e.key === 'Enter') { e.preventDefault(); - if (this.state.showDialog) { + if (this.props.visible) { this._onHideDialog(); } else { this._onShowDialog(e); @@ -340,7 +331,7 @@ class Popover extends Component { if (e.key === 'Escape') { e.preventDefault(); e.stopPropagation(); - if (this.state.showDialog) { + if (this.props.visible) { this._onHideDialog(); } } @@ -373,11 +364,15 @@ class Popover extends Component { * @returns {ReactElement} */ _renderContent() { - const { content } = this.props; + const { content, paddedContent } = this.props; + const className = clsx( + 'popover popupmenu', + paddedContent && 'padded-content' + ); return (
{ content } {!isMobileBrowser() && ( @@ -392,4 +387,18 @@ class Popover extends Component { } } -export default Popover; +/** + * Maps (parts of) the Redux state to the associated {@code Popover}'s props. + * + * @param {Object} state - The Redux state. + * @param {Object} ownProps - The own props of the component. + * @private + * @returns {Props} + */ +function _mapStateToProps(state) { + return { + overflowDrawer: state['features/toolbox'].overflowDrawer + }; +} + +export default connect(_mapStateToProps)(Popover); diff --git a/react/features/connection-indicator/components/web/ConnectionIndicator.js b/react/features/connection-indicator/components/web/ConnectionIndicator.js index 94cfec027..cc3304234 100644 --- a/react/features/connection-indicator/components/web/ConnectionIndicator.js +++ b/react/features/connection-indicator/components/web/ConnectionIndicator.js @@ -109,13 +109,21 @@ type Props = AbstractProps & { t: Function, }; +type State = AbstractState & { + + /** + * Whether popover is ivisible or not. + */ + popoverVisible: boolean +} + /** * Implements a React {@link Component} which displays the current connection * quality percentage and has a popover to show more detailed connection stats. * * @extends {Component} */ -class ConnectionIndicator extends AbstractConnectionIndicator { +class ConnectionIndicator extends AbstractConnectionIndicator { /** * Initializes a new {@code ConnectionIndicator} instance. * @@ -126,10 +134,12 @@ class ConnectionIndicator extends AbstractConnectionIndicator } - disablePopover = { !this.props.enableStatsDisplay } - position = { this.props.statsPopoverPosition }> + participantId = { participantId } /> } + disablePopover = { !enableStatsDisplay } + id = 'participant-connection-indicator' + noPaddingContent = { true } + onPopoverClose = { this._onHidePopover } + onPopoverOpen = { this._onShowPopover } + paddedContent = { true } + position = { statsPopoverPosition } + visible = { this.state.popoverVisible }>
+ style = {{ fontSize: iconSize }}>
{ this._renderIcon() }
@@ -222,6 +239,18 @@ class ConnectionIndicator extends AbstractConnectionIndicator void; + + /** + * Hides popover. + * + * @private + * @returns {void} + */ + _onHidePopover() { + this.setState({ popoverVisible: false }); + } + /** * Creates a ReactElement for displaying an icon that represents the current * connection quality. @@ -282,6 +311,18 @@ class ConnectionIndicator extends AbstractConnectionIndicator ]; } + + _onShowPopover: () => void; + + /** + * Shows popover. + * + * @private + * @returns {void} + */ + _onShowPopover() { + this.setState({ popoverVisible: true }); + } } /** diff --git a/react/features/filmstrip/components/web/Thumbnail.js b/react/features/filmstrip/components/web/Thumbnail.js index c213bd979..7c50a7f41 100644 --- a/react/features/filmstrip/components/web/Thumbnail.js +++ b/react/features/filmstrip/components/web/Thumbnail.js @@ -66,7 +66,12 @@ export type State = {| /** * Indicates whether the thumbnail is hovered or not. */ - isHovered: boolean + isHovered: boolean, + + /** + * Whether popover is visible or not. + */ + popoverVisible: boolean |}; /** @@ -277,18 +282,19 @@ class Thumbnail extends Component { audioLevel: 0, canPlayEventReceived: false, isHovered: false, - displayMode: DISPLAY_VIDEO + displayMode: DISPLAY_VIDEO, + popoverVisible: false }; this.state = { ...state, - displayMode: computeDisplayMode(Thumbnail.getDisplayModeInput(props, state)) + displayMode: computeDisplayMode(Thumbnail.getDisplayModeInput(props, state)), + popoverVisible: false }; this.timeoutHandle = null; this.videoMenuTriggerRef = null; this._clearDoubleClickTimeout = this._clearDoubleClickTimeout.bind(this); - this._setInstance = this._setInstance.bind(this); this._updateAudioLevel = this._updateAudioLevel.bind(this); this._onCanPlay = this._onCanPlay.bind(this); this._onClick = this._onClick.bind(this); @@ -299,7 +305,8 @@ class Thumbnail extends Component { this._onTouchStart = this._onTouchStart.bind(this); this._onTouchEnd = this._onTouchEnd.bind(this); this._onTouchMove = this._onTouchMove.bind(this); - this._showPopupMenu = this._showPopupMenu.bind(this); + this._showPopover = this._showPopover.bind(this); + this._hidePopover = this._hidePopover.bind(this); } /** @@ -503,6 +510,34 @@ class Thumbnail extends Component { }); } + _showPopover: () => void; + + /** + * Shows popover. + * + * @private + * @returns {void} + */ + _showPopover() { + this.setState({ + popoverVisible: true + }); + } + + _hidePopover: () => void; + + /** + * Hides popover. + * + * @private + * @returns {void} + */ + _hidePopover() { + this.setState({ + popoverVisible: false + }); + } + /** * Returns an object with the styles for thumbnail. * @@ -583,19 +618,6 @@ class Thumbnail extends Component { this.setState({ isHovered: false }); } - _showPopupMenu: () => void; - - /** - * Triggers showing the popover context menu. - * - * @returns {void} - */ - _showPopupMenu() { - if (this.videoMenuTriggerRef) { - this.videoMenuTriggerRef.showContextMenu(); - } - } - _onTouchStart: () => void; /** @@ -604,7 +626,7 @@ class Thumbnail extends Component { * @returns {void} */ _onTouchStart() { - this.timeoutHandle = setTimeout(this._showPopupMenu, SHOW_TOOLBAR_CONTEXT_MENU_AFTER); + this.timeoutHandle = setTimeout(this._showPopover, SHOW_TOOLBAR_CONTEXT_MENU_AFTER); if (this._firstTap) { this._clearDoubleClickTimeout(); @@ -860,7 +882,9 @@ class Thumbnail extends Component { + hidePopover = { this._hidePopover } + popoverVisible = { this.state.popoverVisible } + showPopover = { this._showPopover } /> @@ -906,19 +930,6 @@ class Thumbnail extends Component { dispatch(updateLastTrackVideoMediaEvent(jitsiVideoTrack, event.type)); } - _setInstance: Object => void; - - /** - * Stores the local or remote video menu button instance in a variable. - * - * @param {Object} instance - The local or remote video menu trigger instance. - * - * @returns {void} - */ - _setInstance(instance) { - this.videoMenuTriggerRef = instance; - } - /** * Renders a remote participant's 'thumbnail. * @@ -1005,10 +1016,12 @@ class Thumbnail extends Component { + participantID = { id } + popoverVisible = { this.state.popoverVisible } + showPopover = { this._showPopover } /> ); diff --git a/react/features/participants-pane/components/web/MeetingParticipantContextMenu.js b/react/features/participants-pane/components/web/MeetingParticipantContextMenu.js index b70a1593d..2ed9dcda6 100644 --- a/react/features/participants-pane/components/web/MeetingParticipantContextMenu.js +++ b/react/features/participants-pane/components/web/MeetingParticipantContextMenu.js @@ -308,10 +308,9 @@ class MeetingParticipantContextMenu extends Component { * @returns {void} */ _onSendPrivateMessage() { - const { closeDrawer, dispatch, overflowDrawer } = this.props; + const { dispatch } = this.props; dispatch(openChatById(this._getCurrentParticipantId())); - overflowDrawer && closeDrawer(); } _position: () => void; @@ -444,7 +443,7 @@ class MeetingParticipantContextMenu extends Component { ) : ( <> {_isLocalModerator && ( - + <> {overflowDrawer && (_isAudioForceMuted || _isVideoForceMuted) && @@ -481,7 +480,7 @@ class MeetingParticipantContextMenu extends Component { )} - + { _isLocalModerator && ( <> diff --git a/react/features/video-layout/components/TileViewButton.js b/react/features/video-layout/components/TileViewButton.js index 1421f8924..18a46d966 100644 --- a/react/features/video-layout/components/TileViewButton.js +++ b/react/features/video-layout/components/TileViewButton.js @@ -1,5 +1,5 @@ // @flow - +import { batch } from 'react-redux'; import type { Dispatch } from 'redux'; import { @@ -11,6 +11,7 @@ import { translate } from '../../base/i18n'; import { IconTileView } from '../../base/icons'; import { connect } from '../../base/redux'; import { AbstractButton, type AbstractButtonProps } from '../../base/toolbox/components'; +import { setOverflowMenuVisible } from '../../toolbox/actions'; import { setTileView } from '../actions'; import { shouldDisplayTileView } from '../functions'; import logger from '../logger'; @@ -68,7 +69,11 @@ class TileViewButton extends AbstractButton { })); logger.debug(`Tile view ${value ? 'enable' : 'disable'}`); - dispatch(setTileView(value)); + batch(() => { + dispatch(setTileView(value)); + dispatch(setOverflowMenuVisible(false)); + }); + } /** diff --git a/react/features/video-menu/components/web/FlipLocalVideoButton.js b/react/features/video-menu/components/web/FlipLocalVideoButton.js index 2593453c1..a6f130317 100644 --- a/react/features/video-menu/components/web/FlipLocalVideoButton.js +++ b/react/features/video-menu/components/web/FlipLocalVideoButton.js @@ -23,6 +23,11 @@ type Props = { */ dispatch: Function, + /** + * Click handler executed aside from the main action. + */ + onClick?: Function, + /** * Invoked to obtain translated strings. */ @@ -77,8 +82,9 @@ class FlipLocalVideoButton extends PureComponent { * @returns {void} */ _onClick() { - const { _localFlipX, dispatch } = this.props; + const { _localFlipX, dispatch, onClick } = this.props; + onClick && onClick(); dispatch(updateSettings({ localFlipX: !_localFlipX })); diff --git a/react/features/video-menu/components/web/LocalVideoMenuTriggerButton.js b/react/features/video-menu/components/web/LocalVideoMenuTriggerButton.js index 9731b7e65..ca0967564 100644 --- a/react/features/video-menu/components/web/LocalVideoMenuTriggerButton.js +++ b/react/features/video-menu/components/web/LocalVideoMenuTriggerButton.js @@ -38,6 +38,21 @@ type Props = { */ getRef: Function, + /** + * Hides popover. + */ + hidePopover: Function, + + /** + * Whether the popover is visible or not. + */ + popoverVisible: boolean, + + /** + * Shows popover. + */ + showPopover: Function, + /** * The id of the local participant. */ @@ -78,10 +93,6 @@ type Props = { * @extends {Component} */ class LocalVideoMenuTriggerButton extends Component { - /** - * Reference to the Popover instance. - */ - popoverRef: Object; /** * Initializes a new LocalVideoMenuTriggerButton instance. @@ -92,45 +103,10 @@ class LocalVideoMenuTriggerButton extends Component { constructor(props: Props) { super(props); - this.popoverRef = React.createRef(); this._onPopoverClose = this._onPopoverClose.bind(this); this._onPopoverOpen = this._onPopoverOpen.bind(this); } - /** - * Triggers showing the popover's context menu. - * - * @returns {void} - */ - showContextMenu() { - if (this.popoverRef && this.popoverRef.current) { - this.popoverRef.current.showDialog(); - } - } - - /** - * Calls the ref(instance) getter. - * - * @inheritdoc - * @returns {void} - */ - componentDidMount() { - if (this.props.getRef) { - this.props.getRef(this); - } - } - - /** - * Calls the ref(instance) getter. - * - * @inheritdoc - * @returns {void} - */ - componentWillUnmount() { - if (this.props.getRef) { - this.props.getRef(null); - } - } /** * Implements React's {@link Component#render()}. @@ -145,6 +121,8 @@ class LocalVideoMenuTriggerButton extends Component { _showConnectionInfo, _overflowDrawer, _showLocalVideoFlipButton, + hidePopover, + popoverVisible, t } = this.props; @@ -152,7 +130,7 @@ class LocalVideoMenuTriggerButton extends Component { ? : ( - + { isMobileBrowser() && } @@ -163,11 +141,12 @@ class LocalVideoMenuTriggerButton extends Component { isMobileBrowser() || _showLocalVideoFlipButton ? + visible = { popoverVisible }> {!_overflowDrawer && ( @@ -194,7 +173,10 @@ class LocalVideoMenuTriggerButton extends Component { * @returns {void} */ _onPopoverOpen() { - this.props.dispatch(setParticipantContextMenuOpen(true)); + const { dispatch, showPopover } = this.props; + + showPopover(); + dispatch(setParticipantContextMenuOpen(true)); } _onPopoverClose: () => void; @@ -205,8 +187,9 @@ class LocalVideoMenuTriggerButton extends Component { * @returns {void} */ _onPopoverClose() { - const { dispatch } = this.props; + const { hidePopover, dispatch } = this.props; + hidePopover(); batch(() => { dispatch(setParticipantContextMenuOpen(false)); dispatch(renderConnectionStatus(false)); diff --git a/react/features/video-menu/components/web/RemoteVideoMenuTriggerButton.js b/react/features/video-menu/components/web/RemoteVideoMenuTriggerButton.js index a59acf16d..48f2bcd66 100644 --- a/react/features/video-menu/components/web/RemoteVideoMenuTriggerButton.js +++ b/react/features/video-menu/components/web/RemoteVideoMenuTriggerButton.js @@ -1,5 +1,6 @@ // @flow +/* eslint-disable react/jsx-handler-names */ import React, { Component } from 'react'; import { batch } from 'react-redux'; @@ -41,6 +42,21 @@ declare var $: Object; */ type Props = { + /** + * Hides popover. + */ + hidePopover: Function, + + /** + * Whether the popover is visible or not. + */ + popoverVisible: boolean, + + /** + * Shows popover. + */ + showPopover: Function, + /** * Whether or not to display the kick button. */ @@ -128,10 +144,6 @@ type Props = { * @extends {Component} */ class RemoteVideoMenuTriggerButton extends Component { - /** - * Reference to the Popover instance. - */ - popoverRef: Object; /** * Initializes a new RemoteVideoMenuTriggerButton instance. @@ -142,46 +154,10 @@ class RemoteVideoMenuTriggerButton extends Component { constructor(props: Props) { super(props); - this.popoverRef = React.createRef(); this._onPopoverClose = this._onPopoverClose.bind(this); this._onPopoverOpen = this._onPopoverOpen.bind(this); } - /** - * Triggers showing the popover's context menu. - * - * @returns {void} - */ - showContextMenu() { - if (this.popoverRef && this.popoverRef.current) { - this.popoverRef.current.showDialog(); - } - } - - /** - * Calls the ref(instance) getter. - * - * @inheritdoc - * @returns {void} - */ - componentDidMount() { - if (this.props.getRef) { - this.props.getRef(this); - } - } - - /** - * Calls the ref(instance) getter. - * - * @inheritdoc - * @returns {void} - */ - componentWillUnmount() { - if (this.props.getRef) { - this.props.getRef(null); - } - } - /** * Implements React's {@link Component#render()}. * @@ -189,7 +165,13 @@ class RemoteVideoMenuTriggerButton extends Component { * @returns {ReactElement} */ render() { - const { _overflowDrawer, _showConnectionInfo, _participantDisplayName, participantID } = this.props; + const { + _overflowDrawer, + _showConnectionInfo, + _participantDisplayName, + participantID, + popoverVisible + } = this.props; const content = _showConnectionInfo ? : this._renderRemoteVideoMenu(); @@ -203,11 +185,11 @@ class RemoteVideoMenuTriggerButton extends Component { return ( + visible = { popoverVisible }> {!_overflowDrawer && ( {!isMobileBrowser() && { * @returns {void} */ _onPopoverOpen() { - this.props.dispatch(setParticipantContextMenuOpen(true)); + const { dispatch, showPopover } = this.props; + + showPopover(); + dispatch(setParticipantContextMenuOpen(true)); } _onPopoverClose: () => void; @@ -243,8 +228,9 @@ class RemoteVideoMenuTriggerButton extends Component { * @returns {void} */ _onPopoverClose() { - const { dispatch } = this.props; + const { dispatch, hidePopover } = this.props; + hidePopover(); batch(() => { dispatch(setParticipantContextMenuOpen(false)); dispatch(renderConnectionStatus(false)); @@ -271,6 +257,7 @@ class RemoteVideoMenuTriggerButton extends Component { participantID } = this.props; + const actions = []; const buttons = []; const showVolumeSlider = !isIosMobileBrowser() && onVolumeChange @@ -343,7 +330,7 @@ class RemoteVideoMenuTriggerButton extends Component { ); if (isMobileBrowser()) { - buttons.push( + actions.push( @@ -351,7 +338,7 @@ class RemoteVideoMenuTriggerButton extends Component { } if (showVolumeSlider) { - buttons.push( + actions.push( { ); } - if (buttons.length > 0) { + if (buttons.length > 0 || actions.length > 0) { return ( - { buttons } + <> + { buttons.length > 0 + &&
  • +
      + { buttons } +
    +
    • + } + + <> + { actions.length > 0 + &&
  • +
      + {actions} +
    +
    • + } +     ); } @@ -437,3 +441,4 @@ function _mapStateToProps(state, ownProps) { } export default translate(connect(_mapStateToProps)(RemoteVideoMenuTriggerButton)); +/* eslint-enable react/jsx-handler-names */ diff --git a/react/features/video-menu/components/web/VideoMenuButton.js b/react/features/video-menu/components/web/VideoMenuButton.js index f18438625..6d4685e13 100644 --- a/react/features/video-menu/components/web/VideoMenuButton.js +++ b/react/features/video-menu/components/web/VideoMenuButton.js @@ -61,8 +61,7 @@ export default class VideoMenuButton extends Component { /** * KeyPress handler for accessibility. * - * @param {Object} e - The key event to handle. - * + * @param {Object} e - The synthetic event. * @returns {void} */ _onKeyPress(e) {