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

Context-menu repositioned #4194

Merged
merged 20 commits into from
Jul 28, 2021
Merged
Show file tree
Hide file tree
Changes from 12 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
8 changes: 8 additions & 0 deletions src/components/PopoverWithMeasuredContent.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,14 @@ class PopoverWithMeasuredContent extends Component {
) || !_.isEqual(this.state, nextState);
}

componentDidUpdate(prevProps) {
// When Popover is shown recalculate
if (!prevProps.isVisible && this.props.isVisible) {
// eslint-disable-next-line react/no-did-update-set-state
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB, creating a new method for the setState() will also get rid of this warning.

this.setState({isContentMeasured: false});
}
}

/**
* Measure the size of the popover's content.
*
Expand Down
12 changes: 12 additions & 0 deletions src/pages/home/report/ContextMenu/ContextMenuContext.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import React from 'react';

const ContextMenuContextDefaultValue = {
showContextMenu: () => {},
hideContextMenu: () => {},
isActiveReportAction: () => {},
showDeleteConfirmModal: () => {},
};
const ContextMenuContext = React.createContext(ContextMenuContextDefaultValue);
ContextMenuContext.displayName = 'ContextMenuContext';

export {ContextMenuContext, ContextMenuContextDefaultValue};
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import _ from 'underscore';
import React from 'react';
import {View} from 'react-native';
import PropTypes from 'prop-types';
import {
propTypes as ReportActionContextMenuPropsTypes,
defaultProps as ReportActionContextMenuDefaultProps,
} from '../ReportActionContextMenuPropsTypes';
import withLocalize from '../../../../../components/withLocalize';
import {getMiniReportActionContextMenuWrapperStyle} from '../../../../../styles/getReportActionItemStyles';
import ReportActionContextMenu from '../ReportActionContextMenu';

const propTypes = {
..._.omit(ReportActionContextMenuPropsTypes, ['isMini']),
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there other props that are not relevant to the MiniReportActionContextMenu? hidePopover, for example? Also, I think generally it's better to avoid the _.omit(GenericPropTypes, ['specificProp']) pattern. Instead, isMini should not be included in ReportActionContextMenuPropTypes. It should be included inline in BaseReportActionContextMenu.


/** Should the comment have the appearance of being grouped with the previous comment? */
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
/** Should the comment have the appearance of being grouped with the previous comment? */
/** Should the reportAction this menu is attached to have the appearance of being grouped with the previous reportAction? */

displayAsGroup: PropTypes.bool,
};

const defaultProps = {
..._.omit(ReportActionContextMenuDefaultProps, ['isMini']),
displayAsGroup: false,
};

const MiniReportActionContextMenu = props => (
<View style={getMiniReportActionContextMenuWrapperStyle(props.displayAsGroup)}>
{/* eslint-disable-next-line react/jsx-props-no-spreading */}
<ReportActionContextMenu isMini {...props} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Elsewhere in the repo, when we have multiple wrapper implementations around a base component, we prefix the base component with Base, which I think makes it a bit clearer what its role is, and that it's not really meant for export. So I think we should change ReportActionContextMenu -> BaseReportActionContextMenu.

</View>
);

MiniReportActionContextMenu.propTypes = propTypes;
MiniReportActionContextMenu.defaultProps = defaultProps;
MiniReportActionContextMenu.displayName = 'MiniReportActionContextMenu';

export default withLocalize(MiniReportActionContextMenu);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we wrapping this in a withLocalize()? Seems we are not translating anything.

Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default () => null;
258 changes: 258 additions & 0 deletions src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,258 @@
import React from 'react';
import {
Dimensions,
} from 'react-native';
import PropTypes from 'prop-types';
import _ from 'underscore';
import {
deleteReportComment,
} from '../../../../libs/actions/Report';
import withLocalize, {withLocalizePropTypes} from '../../../../components/withLocalize';
import PopoverWithMeasuredContent from '../../../../components/PopoverWithMeasuredContent';
import ReportActionContextMenu from './ReportActionContextMenu';
import ConfirmModal from '../../../../components/ConfirmModal';

const propTypes = {
/** Update the callbacks on ContextMenu Context */
setValue: PropTypes.func.isRequired,
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
setValue: PropTypes.func.isRequired,
updateContext: PropTypes.func.isRequired,

...withLocalizePropTypes,
};

class PopoverReportActionContextMenu extends React.Component {
constructor(props) {
super(props);

this.state = {
reportID: 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should default to null when not present instead of 0.

Suggested change
reportID: 0,
reportID: null,

reportAction: {},
selection: '',
reportActionDraftMessage: '',
isPopoverVisible: false,
isDeleteCommentConfirmModalVisible: false,
cursorRelativePosition: {
horizontal: 0,
vertical: 0,
},

// The horizontal and vertical position (relative to the screen) where the popover will display.
popoverAnchorPosition: {
horizontal: 0,
vertical: 0,
},
};
this.onPopoverHide = () => {};
this.contextMenuAchor = undefined;
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
this.contextMenuAchor = undefined;
this.contextMenuAnchor = undefined;

this.showContextMenu = this.showContextMenu.bind(this);
this.hideContextMenu = this.hideContextMenu.bind(this);
this.measureContent = this.measureContent.bind(this);
this.measureContextMenuAnchorPosition = this.measureContextMenuAnchorPosition.bind(this);
this.confirmDeleteAndHideModal = this.confirmDeleteAndHideModal.bind(this);
this.hideDeleteConfirmModal = this.hideDeleteConfirmModal.bind(this);
this.showDeleteConfirmModal = this.showDeleteConfirmModal.bind(this);
this.contextMenuHidden = this.contextMenuHidden.bind(this);
this.getContextMenuMeasuredLocation = this.getContextMenuMeasuredLocation.bind(this);
this.isActiveReportAction = this.isActiveReportAction.bind(this);
}

componentDidMount() {
this.props.setValue({
showContextMenu: this.showContextMenu,
hideContextMenu: this.hideContextMenu,
isActiveReportAction: this.isActiveReportAction,
showDeleteConfirmModal: this.showDeleteConfirmModal,
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this is very confusing. But I want to suggest it's maybe not a good use of context because...

  • The component only mounts once
  • It sets the value of the context - so it should maybe be a provider (if anything)?
  • But the fact that the values don't change begs the question of whether to use context at all

The value in having a context is that something will update it in the future and the consumers will automatically be updated. In this case, we are just setting some methods here so we can call them from other places.

It may be fine to share some methods with a consumer wrapped in the provider, but we should create them and pass them once instead of doing this prop callback that sets to state creating an immediate re-render?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. I will drop context and use Callbacks. We can export all the required callbacks to an external lib/module and call them.
I will reply or act upon the other comments when this is done.

Dimensions.addEventListener('change', this.measureContextMenuAnchorPosition);
}

shouldComponentUpdate(nextProps, nextState) {
return this.state.isPopoverVisible !== nextState.isPopoverVisible
|| this.state.popoverAnchorPosition !== nextState.popoverAnchorPosition
|| this.state.isDeleteCommentConfirmModalVisible !== nextState.isDeleteCommentConfirmModalVisible;
}

componentWillUnmount() {
Dimensions.removeEventListener('change', this.measureContextMenuAnchorPosition);
}

/**
* Get the Context menu anchor position
* We calculate the achor coordinates from measureInWindow async method
*
* @returns {Promise<Object>}
* @memberof PopoverReportActionContextMenu
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's get rid of @memberof? We can see what this is a member of since the class name is right there in the file.

*/
getContextMenuMeasuredLocation() {
return new Promise((res) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB:

Suggested change
return new Promise((res) => {
return new Promise((resolve) => {

if (this.contextMenuAchor) {
this.contextMenuAchor.measureInWindow((x, y) => res({x, y}));
} else {
res({x: 0, y: 0});
}
});
}

isActiveReportAction(actionId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

need JS doc for the param and return value

Copy link
Contributor

Choose a reason for hiding this comment

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

Also should be actionID instead of actionId

return this.state.reportAction.reportActionID === actionId;
}

/**
* Show the ReportActionContextMenu modal popover.
*
* @param {Object} [event] - A press event.
* @param {string} [selection] - A copy text.
* @param {Element} contextMenuAnchor - popoverAnchor
* @param {Number} reportID - Active Report Id
* @param {Object} reportAction - ReportAction for ContextMenu
* @param {String} draftMessage - ReportAction Draftmessage
* @param {Function} [onShown=() => {}] - Run a callback when Menu is shown
* @memberof PopoverReportActionContextMenu
*/
showContextMenu(event, selection, contextMenuAnchor, reportID, reportAction, draftMessage, onShown = () => {}) {
const nativeEvent = event.nativeEvent || {};
this.contextMenuAchor = contextMenuAnchor;
this.getContextMenuMeasuredLocation().then(({x, y}) => {
this.setState({
cursorRelativePosition: {
horizontal: nativeEvent.pageX - x,
vertical: nativeEvent.pageY - y,
},
popoverAnchorPosition: {
horizontal: nativeEvent.pageX,
vertical: nativeEvent.pageY,
},
reportID,
reportAction,
selection,
isPopoverVisible: true,
reportActionDraftMessage: draftMessage,
}, onShown);
});
}

/**
* This gets called on Dimensions change to find the anchor coordinates for the action context menu.
*/
measureContextMenuAnchorPosition() {
if (!this.state.isPopoverVisible) {
return;
}
this.getContextMenuMeasuredLocation().then(({x, y}) => {
if (!x || !y) {
return;
}
this.setState(prev => ({
popoverAnchorPosition: {
horizontal: prev.cursorRelativePosition.horizontal + x,
vertical: prev.cursorRelativePosition.vertical + y,
},
}));
});
}

contextMenuHidden() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function should be named after what it does, not the event when it's used.

this.onPopoverHide();

// After we have called the action, reset it.
this.onPopoverHide = () => {};
}

/**
* Hide the ReportActionContextMenu modal popover.
* @param {Function} onHideCallback Callback to be called after popover is completely hidden
*/
hideContextMenu(onHideCallback) {
if (_.isFunction(onHideCallback)) {
this.onPopoverHide = onHideCallback;
}
this.setState({
reportID: 0,
reportAction: {},
selection: '',
reportActionDraftMessage: '',
isPopoverVisible: false,
});
}

/**
* Used to calculate the Context Menu Dimensions
*
* @returns {JSX}
* @memberof PopoverReportActionContextMenu
*/
measureContent() {
return (
<ReportActionContextMenu
isVisible
selection={this.state.selection}
reportID={this.state.reportID}
reportAction={this.state.reportAction}
hidePopover={this.hideContextMenu}
showDeleteConfirmModal={this.showDeleteConfirmModal}
/>
);
}

confirmDeleteAndHideModal() {
deleteReportComment(this.state.reportID, this.state.reportAction);
this.setState({isDeleteCommentConfirmModalVisible: false});
}

hideDeleteConfirmModal() {
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
hideDeleteConfirmModal() {
hideDeleteModal() {

this.setState({
reportID: 0,
reportAction: {},
isDeleteCommentConfirmModalVisible: false,
});
}

/**
* Opens the Confirm delete action modal
* @param {Number} reportID
* @param {Object} reportAction
* @memberof PopoverReportActionContextMenu
*/
showDeleteConfirmModal(reportID, reportAction) {
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
showDeleteConfirmModal(reportID, reportAction) {
showDeleteModal(reportID, reportAction) {

this.setState({reportID, reportAction, isDeleteCommentConfirmModalVisible: true});
}

render() {
return (
<>
<PopoverWithMeasuredContent
isVisible={this.state.isPopoverVisible}
onClose={this.hideContextMenu}
onModalHide={this.contextMenuHidden}
anchorPosition={this.state.popoverAnchorPosition}
animationIn="fadeIn"
animationOutTiming={1}
measureContent={this.measureContent}
shouldSetModalVisibility={false}
fullscreen={false}
>
<ReportActionContextMenu
isVisible
reportID={this.state.reportID}
reportAction={this.state.reportAction}
draftMessage={this.state.reportActionDraftMessage}
hidePopover={this.hideContextMenu}
showDeleteConfirmModal={this.showDeleteConfirmModal}
/>
</PopoverWithMeasuredContent>
<ConfirmModal
title={this.props.translate('reportActionContextMenu.deleteComment')}
isVisible={this.state.isDeleteCommentConfirmModalVisible}
onConfirm={this.confirmDeleteAndHideModal}
onCancel={this.hideDeleteConfirmModal}
prompt={this.props.translate('reportActionContextMenu.deleteConfirmation')}
confirmText={this.props.translate('common.delete')}
cancelText={this.props.translate('common.cancel')}
/>
marcaaron marked this conversation as resolved.
Show resolved Hide resolved
</>
);
}
}

PopoverReportActionContextMenu.propTypes = propTypes;
PopoverReportActionContextMenu.displayName = 'PopoverReportActionContextMenu';

export default withLocalize(PopoverReportActionContextMenu);
Loading