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

Add Context menu with Pin option to LHN #16079

Merged
merged 42 commits into from
Jun 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
fc9aec9
changing branchs
Gonals Mar 15, 2023
5bbf1db
Merge branch 'main' into alberto-pin
Gonals Mar 16, 2023
ffaf634
add PIN action
Gonals Mar 16, 2023
8789f75
correctly show menu
Gonals Mar 17, 2023
d3befa3
handle pin/unpin behavior
Gonals Mar 17, 2023
28079fe
no need fur success messages
Gonals Mar 17, 2023
c3c45ce
figure out what's going on
Gonals Mar 17, 2023
a6e1aec
make pin/unpin work fine
Gonals Mar 17, 2023
6b26871
cleanup
Gonals Mar 17, 2023
cc83d47
lint
Gonals Mar 17, 2023
6d65fb8
more lint cleanup
Gonals Mar 17, 2023
96c8256
update test
Gonals Mar 17, 2023
eafa87f
handle mobile devices
Gonals Mar 18, 2023
310ae83
Merge branch 'main' into alberto-pin
Gonals Mar 22, 2023
315fbc6
Use pressablewithSecondaryInteraction instead of touchableOpacity
Gonals Mar 22, 2023
7ef10b8
Turn Pressable into touchableOpacity
Gonals Mar 22, 2023
5803c47
cleanUp
Gonals Mar 22, 2023
9f4349d
update test
Gonals Mar 22, 2023
233ae47
Merge branch 'main' into alberto-pin
Gonals Apr 4, 2023
223a550
add better defaults
Gonals Apr 4, 2023
294ed23
Merge branch 'main' into alberto-pin
Gonals Apr 5, 2023
95de7c2
remove unneeded props
Gonals Apr 5, 2023
d4c619e
Merge branch 'main' into alberto-pin
Gonals Apr 10, 2023
7263848
conflicts
Gonals May 16, 2023
0c827dc
Merge branch 'main' into alberto-pin
Gonals May 16, 2023
b554112
more conflicts
Gonals May 16, 2023
fb6c9b4
back to pressable
Gonals May 16, 2023
cc01e7e
pass params better
Gonals May 16, 2023
8c45c6f
lint
Gonals May 17, 2023
7f61b2a
prettier
Gonals May 17, 2023
3775be7
conflicts
Gonals May 18, 2023
a0b72c6
Merge branch 'main' into alberto-pin
Gonals May 23, 2023
f3c9046
close popup if pin changes
Gonals May 23, 2023
c1a93de
remove unneeded import
Gonals May 23, 2023
beaa0e0
Merge branch 'main' into alberto-pin
Gonals May 26, 2023
698b876
remove measure
Gonals May 26, 2023
9e7ce9b
Merge branch 'main' into alberto-pin
Gonals May 29, 2023
9cab709
use pressable
Gonals May 29, 2023
f125e5d
conflicts
Gonals May 30, 2023
fddded2
conflicts
Gonals Jun 1, 2023
137631e
remove unused param
Gonals Jun 1, 2023
a433efb
prettier
Gonals Jun 1, 2023
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
44 changes: 38 additions & 6 deletions src/components/LHNOptionsList/OptionRowLHN.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import _ from 'underscore';
import React from 'react';
import PropTypes from 'prop-types';
import {TouchableOpacity, View, StyleSheet} from 'react-native';
import {View, StyleSheet} from 'react-native';
import * as optionRowStyles from '../../styles/optionRowStyles';
import styles from '../../styles/styles';
import * as StyleUtils from '../../styles/StyleUtils';
Expand All @@ -19,6 +19,9 @@ import themeColors from '../../styles/themes/default';
import SidebarUtils from '../../libs/SidebarUtils';
import TextPill from '../TextPill';
import OfflineWithFeedback from '../OfflineWithFeedback';
import PressableWithSecondaryInteraction from '../PressableWithSecondaryInteraction';
import * as ReportActionContextMenu from '../../pages/home/report/ContextMenu/ReportActionContextMenu';
import * as ContextMenuActions from '../../pages/home/report/ContextMenu/ContextMenuActions';

const propTypes = {
/** Style for hovered state */
Expand Down Expand Up @@ -52,11 +55,16 @@ const defaultProps = {

const OptionRowLHN = (props) => {
const optionItem = SidebarUtils.getOptionData(props.reportID);

React.useEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm actually not seeing why this is necessary. I removed this and everything seems to work fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

That discussion started from here.
Finally landed on #16079 (comment)

ReportActionContextMenu.hideContextMenu(false);
}, [optionItem.isPinned]);
Copy link
Contributor

Choose a reason for hiding this comment

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

@Gonals we should add null safety check on this line. Sometimes app crashes when logout because optionItem is undefined returning here:

// When a user signs out, Onyx is cleared. Due to the lazy rendering with a virtual list, it's possible for
// this method to be called after the Onyx data has been cleared out. In that case, it's fine to do
// a null check here and return early.
if (!report || !personalDetails) {
return;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I faced this today

Screenshot 2023-06-05 at 7 53 31 AM


if (!optionItem) {
return null;
}

let touchableRef = null;
let popoverAnchor = null;
const textStyle = props.isFocused ? styles.sidebarLinkActiveText : styles.sidebarLinkText;
const textUnreadStyle = optionItem.isUnread ? [textStyle, styles.sidebarLinkTextBold] : [textStyle];
const displayNameStyle = StyleUtils.combineStyles([styles.optionDisplayName, styles.optionDisplayNameCompact, styles.pre, ...textUnreadStyle], props.style);
Expand All @@ -83,6 +91,28 @@ const OptionRowLHN = (props) => {
// If the item is a thread within a workspace, we will show the subtitle as the second line instead of in a pill
const alternativeText = optionItem.isThread && optionItem.subtitle ? optionItem.subtitle : optionItem.alternateText;

/**
* Show the ReportActionContextMenu modal popover.
*
* @param {Object} [event] - A press event.
*/
const showPopover = (event) => {
ReportActionContextMenu.showContextMenu(
ContextMenuActions.CONTEXT_MENU_TYPES.REPORT,
event,
'',
popoverAnchor,
props.reportID,
{},
'',
() => {},
() => {},
false,
false,
optionItem.isPinned,
);
};

return (
<OfflineWithFeedback
pendingAction={optionItem.pendingAction}
Expand All @@ -91,15 +121,17 @@ const OptionRowLHN = (props) => {
>
<Hoverable>
Copy link
Contributor

@0xmiros 0xmiros Apr 4, 2023

Choose a reason for hiding this comment

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

Let's use same pattern as others. What's the difference (pros/cons) between Hoverable inside PressableWithSecondaryInteraction and PressableWithSecondaryInteraction inside Hoverable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have hovered available for styling purposes. I think that's the main reason I have it this way

{(hovered) => (
<TouchableOpacity
ref={(el) => (touchableRef = el)}
<PressableWithSecondaryInteraction
ref={(el) => (popoverAnchor = el)}
onPress={(e) => {
if (e) {
e.preventDefault();
}

props.onSelectRow(optionItem, touchableRef);
props.onSelectRow(optionItem, popoverAnchor);
}}
onSecondaryInteraction={(e) => showPopover(e)}
withoutFocusOnSecondaryInteraction
activeOpacity={0.8}
style={[
styles.flexRow,
Expand Down Expand Up @@ -213,7 +245,7 @@ const OptionRowLHN = (props) => {
</View>
)}
</View>
</TouchableOpacity>
</PressableWithSecondaryInteraction>
)}
</Hoverable>
</OfflineWithFeedback>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ class PressableWithSecondaryInteraction extends Component {
style={StyleUtils.combineStyles(this.props.inline ? styles.dInline : this.props.style)}
onPressIn={this.props.onPressIn}
onLongPress={this.props.onSecondaryInteraction ? this.executeSecondaryInteraction : undefined}
activeOpacity={this.props.activeOpacity}
onPressOut={this.props.onPressOut}
onPress={this.props.onPress}
ref={(el) => (this.pressableRef = el)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ const PressableWithSecondaryInteraction = (props) => {
}}
onPressIn={props.onPressIn}
onPressOut={props.onPressOut}
activeOpacity={props.activeOpacity}
// eslint-disable-next-line react/jsx-props-no-spreading
{..._.omit(props, 'onLongPress')}
>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ const propTypes = {
*
* - No support for delayLongPress.
* - No support for pressIn and pressOut events.
* - No support for opacity
*
* Note: Web uses styling instead of Text due to no support of LongPress. Thus above pointers are not valid for web.
*/
Expand All @@ -37,6 +38,9 @@ const propTypes = {
/** Disable focus trap for the element on secondary interaction */
withoutFocusOnSecondaryInteraction: PropTypes.bool,

/** Opacity to reduce to when active */
activeOpacity: PropTypes.number,

/** Used to apply styles to the Pressable */
style: stylePropTypes,
};
Expand All @@ -48,6 +52,7 @@ const defaultProps = {
preventDefaultContextMenu: true,
inline: false,
withoutFocusOnSecondaryInteraction: false,
activeOpacity: 1,
enableLongPressWithHover: false,
};

Expand Down
11 changes: 6 additions & 5 deletions src/libs/actions/Report.js
Original file line number Diff line number Diff line change
Expand Up @@ -686,24 +686,25 @@ function markCommentAsUnread(reportID, reportActionCreated) {
/**
* Toggles the pinned state of the report.
*
* @param {Object} report
* @param {Object} reportID
* @param {Boolean} isPinnedChat
*/
function togglePinnedState(report) {
const pinnedValue = !report.isPinned;
function togglePinnedState(reportID, isPinnedChat) {
const pinnedValue = !isPinnedChat;

// Optimistically pin/unpin the report before we send out the command
const optimisticData = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${report.reportID}`,
key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
value: {isPinned: pinnedValue},
},
];

API.write(
'TogglePinnedChat',
{
reportID: report.reportID,
reportID,
pinnedValue,
},
{optimisticData},
Expand Down
2 changes: 1 addition & 1 deletion src/pages/home/HeaderView.js
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ const HeaderView = (props) => {
)}
<Tooltip text={props.report.isPinned ? props.translate('common.unPin') : props.translate('common.pin')}>
<Pressable
onPress={Session.checkIfActionIsAllowed(() => Report.togglePinnedState(props.report))}
onPress={Session.checkIfActionIsAllowed(() => Report.togglePinnedState(props.report.reportID, props.report.isPinned))}
style={[styles.touchableButtonImage]}
>
<Icon
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ class BaseReportActionContextMenu extends React.Component {
this.props.anchor,
this.props.isChronosReport,
this.props.reportID,
this.props.isPinnedChat,
);

/**
Expand Down
25 changes: 25 additions & 0 deletions src/pages/home/report/ContextMenu/ContextMenuActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ const CONTEXT_MENU_TYPES = {
LINK: 'LINK',
REPORT_ACTION: 'REPORT_ACTION',
EMAIL: 'EMAIL',
REPORT: 'REPORT',
};

// A list of all the context actions in this menu.
Expand Down Expand Up @@ -266,6 +267,30 @@ export default [
},
getDescription: () => {},
},
{
textTranslateKey: 'common.pin',
icon: Expensicons.Pin,
shouldShow: (type, reportAction, isArchivedRoom, betas, anchor, isChronosReport, reportID, isPinnedChat) => type === CONTEXT_MENU_TYPES.REPORT && !isPinnedChat,
onPress: (closePopover, {reportID}) => {
Report.togglePinnedState(reportID, false);
if (closePopover) {
hideContextMenu(false);
}
},
Comment on lines +273 to +279
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth it to DRY this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean avoiding having two for pin/unpin? I tried a few options and even asked other engineers during ECX. Apparently, this is the way 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I noticed shouldShow is the same for both, and onPress is similar but with the opposite bool values, but I'm good with this as is

getDescription: () => {},
},
{
textTranslateKey: 'common.unPin',
icon: Expensicons.Pin,
shouldShow: (type, reportAction, isArchivedRoom, betas, anchor, isChronosReport, reportID, isPinnedChat) => type === CONTEXT_MENU_TYPES.REPORT && isPinnedChat,
Copy link
Member

Choose a reason for hiding this comment

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

Pin/UnPin options should not be visible on MoneyRequest chats as they are not pinnable. #20255

onPress: (closePopover, {reportID}) => {
Report.togglePinnedState(reportID, true);
if (closePopover) {
hideContextMenu(false);
}
},
getDescription: () => {},
},
{
textTranslateKey: 'reportActionContextMenu.flagAsOffensive',
icon: Expensicons.Flag,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ class PopoverReportActionContextMenu extends React.Component {
},
isArchivedRoom: false,
isChronosReport: false,
isPinnedChat: false,
};
this.onPopoverShow = () => {};
this.onPopoverHide = () => {};
Expand Down Expand Up @@ -126,9 +127,22 @@ class PopoverReportActionContextMenu extends React.Component {
* @param {Function} [onHide] - Run a callback when Menu is hidden
* @param {Boolean} isArchivedRoom - Whether the provided report is an archived room
* @param {Boolean} isChronosReport - Flag to check if the chat participant is Chronos
* @param {String} childReportID - ReportAction childReportID
* @param {Boolean} isPinnedChat - Flag to check if the chat is pinned in the LHN. Used for the Pin/Unpin action
*/
showContextMenu(type, event, selection, contextMenuAnchor, reportID, reportAction, draftMessage, onShow = () => {}, onHide = () => {}, isArchivedRoom, isChronosReport) {
showContextMenu(
type,
event,
selection,
contextMenuAnchor,
reportID,
reportAction,
draftMessage,
onShow = () => {},
onHide = () => {},
isArchivedRoom = false,
isChronosReport = false,
isPinnedChat = false,
) {
const nativeEvent = event.nativeEvent || {};
this.contextMenuAnchor = contextMenuAnchor;
this.contextMenuTargetNode = nativeEvent.target;
Expand Down Expand Up @@ -158,6 +172,7 @@ class PopoverReportActionContextMenu extends React.Component {
reportActionDraftMessage: draftMessage,
isArchivedRoom,
isChronosReport,
isPinnedChat,
});
});
}
Expand Down Expand Up @@ -246,6 +261,7 @@ class PopoverReportActionContextMenu extends React.Component {
shouldSetModalVisibilityForDeleteConfirmation: true,
isArchivedRoom: false,
isChronosReport: false,
isPinnedChat: false,
});
}

Expand Down Expand Up @@ -292,6 +308,7 @@ class PopoverReportActionContextMenu extends React.Component {
selection={this.state.selection}
isArchivedRoom={this.state.isArchivedRoom}
isChronosReport={this.state.isChronosReport}
isPinnedChat={this.state.isPinnedChat}
anchor={this.contextMenuTargetNode}
contentRef={this.contentRef}
/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ const contextMenuRef = React.createRef();
/**
* Show the ReportActionContextMenu modal popover.
*
* @param {string} type - the context menu type to display [EMAIL, LINK, REPORT_ACTION]
* @param {string} type - the context menu type to display [EMAIL, LINK, REPORT_ACTION, REPORT]
* @param {Object} [event] - A press event.
* @param {String} [selection] - Copied content.
* @param {Element} contextMenuAnchor - popoverAnchor
Expand All @@ -16,7 +16,7 @@ const contextMenuRef = React.createRef();
* @param {Function} [onHide=() => {}] - Run a callback when Menu is hidden
* @param {Boolean} isArchivedRoom - Whether the provided report is an archived room
* @param {Boolean} isChronosReport - Flag to check if the chat participant is Chronos
* @param {String} childReportID - The child report (thread) of this action
* @param {Boolean} isPinnedChat - Flag to check if the chat is pinned in the LHN. Used for the Pin/Unpin action
*/
function showContextMenu(
type,
Expand All @@ -30,12 +30,12 @@ function showContextMenu(
onHide = () => {},
isArchivedRoom = false,
isChronosReport = false,
childReportID = '0',
isPinnedChat = false,
) {
if (!contextMenuRef.current) {
return;
}
contextMenuRef.current.showContextMenu(type, event, selection, contextMenuAnchor, reportID, reportAction, draftMessage, onShow, onHide, isArchivedRoom, isChronosReport, childReportID);
contextMenuRef.current.showContextMenu(type, event, selection, contextMenuAnchor, reportID, reportAction, draftMessage, onShow, onHide, isArchivedRoom, isChronosReport, isPinnedChat);
Gonals marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand Down
1 change: 0 additions & 1 deletion src/pages/home/report/ReportActionItem.js
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,6 @@ function ReportActionItem(props) {
toggleContextMenuFromActiveReportAction,
ReportUtils.isArchivedRoom(props.report),
ReportUtils.chatIncludesChronos(props.report),
props.action.childReportID,
);
},
[props.draftMessage, props.action, props.report, toggleContextMenuFromActiveReportAction],
Expand Down
6 changes: 1 addition & 5 deletions tests/actions/ReportTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -125,10 +125,6 @@ describe('actions/Report', () => {
const TEST_USER_ACCOUNT_ID = 1;
const TEST_USER_LOGIN = 'test@test.com';
const REPORT_ID = '1';
const REPORT = {
reportID: REPORT_ID,
isPinned: false,
};

let reportIsPinned;
Onyx.connect({
Expand All @@ -139,7 +135,7 @@ describe('actions/Report', () => {
// Set up Onyx with some test user data
return TestHelper.signInWithTestUser(TEST_USER_ACCOUNT_ID, TEST_USER_LOGIN)
.then(() => {
Report.togglePinnedState(REPORT);
Report.togglePinnedState(REPORT_ID, false);
return waitForPromisesToResolve();
})
.then(() => {
Expand Down