-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Refactor reportAction of context menu #26741
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
Changes from all commits
5005161
068cc42
2c3260a
037ce5f
c30867c
4fa87ac
017c714
88eaa80
14c6745
985c5c2
05eae4e
ed1a7ec
50120ef
88bd1b6
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 |
|---|---|---|
| @@ -1,7 +1,9 @@ | ||
| import React, {useState} from 'react'; | ||
| import React, {useMemo, useState, memo} from 'react'; | ||
| import {InteractionManager, View} from 'react-native'; | ||
| import lodashGet from 'lodash/get'; | ||
| import _ from 'underscore'; | ||
| import PropTypes from 'prop-types'; | ||
| import {withOnyx} from 'react-native-onyx'; | ||
| import getReportActionContextMenuStyles from '../../../../styles/getReportActionContextMenuStyles'; | ||
| import ContextMenuItem from '../../../../components/ContextMenuItem'; | ||
| import {propTypes as genericReportActionContextMenuPropTypes, defaultProps as GenericReportActionContextMenuDefaultProps} from './genericReportActionContextMenuPropTypes'; | ||
|
|
@@ -12,6 +14,7 @@ import withWindowDimensions, {windowDimensionsPropTypes} from '../../../../compo | |
| import {withBetas} from '../../../../components/OnyxProvider'; | ||
| import * as Session from '../../../../libs/actions/Session'; | ||
| import {hideContextMenu} from './ReportActionContextMenu'; | ||
| import ONYXKEYS from '../../../../ONYXKEYS'; | ||
|
|
||
| const propTypes = { | ||
| /** String representing the context menu type [LINK, REPORT_ACTION] which controls context menu choices */ | ||
|
|
@@ -44,18 +47,14 @@ const defaultProps = { | |
| function BaseReportActionContextMenu(props) { | ||
| const [shouldKeepOpen, setShouldKeepOpen] = useState(false); | ||
| const wrapperStyle = getReportActionContextMenuStyles(props.isMini, props.isSmallScreenWidth); | ||
|
|
||
| const reportAction = useMemo(() => { | ||
| if (_.isEmpty(props.reportActions) || props.reportActionID === '0') return {}; | ||
| return props.reportActions[props.reportActionID] || {}; | ||
| }, [props.reportActions, props.reportActionID]); | ||
|
|
||
| const shouldShowFilter = (contextAction) => | ||
| contextAction.shouldShow( | ||
| props.type, | ||
| props.reportAction, | ||
| props.isArchivedRoom, | ||
| props.betas, | ||
| props.anchor, | ||
| props.isChronosReport, | ||
| props.reportID, | ||
| props.isPinnedChat, | ||
| props.isUnreadChat, | ||
| ); | ||
| contextAction.shouldShow(props.type, reportAction, props.isArchivedRoom, props.betas, props.anchor, props.isChronosReport, props.reportID, props.isPinnedChat, props.isUnreadChat); | ||
|
|
||
| /** | ||
| * Checks if user is anonymous. If true and the action doesn't accept for anonymous user, hides the context menu and | ||
|
|
@@ -85,7 +84,7 @@ function BaseReportActionContextMenu(props) { | |
| {_.map(_.filter(ContextMenuActions, shouldShowFilter), (contextAction) => { | ||
| const closePopup = !props.isMini; | ||
| const payload = { | ||
| reportAction: props.reportAction, | ||
| reportAction, | ||
| reportID: props.reportID, | ||
| draftMessage: props.draftMessage, | ||
| selection: props.selection, | ||
|
|
@@ -106,7 +105,7 @@ function BaseReportActionContextMenu(props) { | |
| return ( | ||
| <ContextMenuItem | ||
| icon={contextAction.icon} | ||
| text={props.translate(contextAction.textTranslateKey, {action: props.reportAction})} | ||
| text={props.translate(contextAction.textTranslateKey, {action: reportAction})} | ||
| successIcon={contextAction.successIcon} | ||
| successText={contextAction.successTextTranslateKey ? props.translate(contextAction.successTextTranslateKey) : undefined} | ||
| isMini={props.isMini} | ||
|
|
@@ -125,4 +124,25 @@ function BaseReportActionContextMenu(props) { | |
| BaseReportActionContextMenu.propTypes = propTypes; | ||
| BaseReportActionContextMenu.defaultProps = defaultProps; | ||
|
|
||
| export default compose(withLocalize, withBetas(), withWindowDimensions)(BaseReportActionContextMenu); | ||
| export default compose( | ||
| withLocalize, | ||
| withBetas(), | ||
| withWindowDimensions, | ||
| withOnyx({ | ||
| reportActions: { | ||
| key: ({originalReportID}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${originalReportID}`, | ||
| canEvict: false, | ||
| }, | ||
| }), | ||
| )( | ||
| memo(BaseReportActionContextMenu, (prevProps, nextProps) => { | ||
|
Contributor
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 you please add code comments to explain why this memo is here and what it's purpose is? |
||
| const prevReportAction = lodashGet(prevProps.reportActions, prevProps.reportActionID, ''); | ||
| const nextReportAction = lodashGet(nextProps.reportActions, nextProps.reportActionID, ''); | ||
|
|
||
| // We only want to re-render when the report action that is attached to is changed | ||
| if (prevReportAction !== nextReportAction) { | ||
| return false; | ||
| } | ||
| return _.isEqual(_.omit(prevProps, 'reportActions'), _.omit(nextProps, 'reportActions')); | ||
| }), | ||
| ); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,14 @@ | ||
| import PropTypes from 'prop-types'; | ||
| import reportActionPropTypes from '../reportActionPropTypes'; | ||
|
|
||
| const propTypes = { | ||
| /** The ID of the report this report action is attached to. */ | ||
| reportID: PropTypes.string.isRequired, | ||
|
|
||
| /** The report action this context menu is attached to. */ | ||
| reportAction: PropTypes.shape(reportActionPropTypes).isRequired, | ||
| /** The ID of the report action this context menu is attached to. */ | ||
| reportActionID: PropTypes.string.isRequired, | ||
|
|
||
| /** The ID of the original report from which the given reportAction is first created. */ | ||
| originalReportID: PropTypes.string.isRequired, | ||
|
Contributor
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. Just need a bit of clarity on this one. Can you explain (maybe with an example) what you mean when you say in your proposal "The thread first chat originalReportID and reportID is different."?
Contributor
Author
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. The thread first chat is the parent report action. When we pass the
Contributor
Author
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. Screencast.from.07-09-2023.22.07.10.webmAs you can see in the video, the first message in the thread is the thread first chat.
Contributor
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. Thanks, I think I get what you mean now! Maybe a clearer name would be
Contributor
Author
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. @jjcoffee Other places in the App also use this name. And if the reportAction isn't the thread first chat
Contributor
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. Ah ignore me, I see that we're using the same naming elsewhere! |
||
|
|
||
| /** If true, this component will be a small, row-oriented menu that displays icons but not text. | ||
| If false, this component will be a larger, column-oriented menu that displays icons alongside text in each row. */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -223,15 +223,16 @@ function ReportActionItem(props) { | |
| selection, | ||
| popoverAnchorRef, | ||
| props.report.reportID, | ||
| props.action, | ||
| props.action.reportActionID, | ||
| originalReportID, | ||
| props.draftMessage, | ||
| () => {}, | ||
| toggleContextMenuFromActiveReportAction, | ||
| ReportUtils.isArchivedRoom(originalReport), | ||
| ReportUtils.chatIncludesChronos(originalReport), | ||
| ); | ||
| }, | ||
| [props.draftMessage, props.action, props.report.reportID, toggleContextMenuFromActiveReportAction, originalReport], | ||
| [props.draftMessage, props.action, props.report.reportID, toggleContextMenuFromActiveReportAction, originalReport, originalReportID], | ||
| ); | ||
|
|
||
| const toggleReaction = useCallback( | ||
|
|
@@ -561,8 +562,9 @@ function ReportActionItem(props) { | |
| {props.shouldDisplayNewMarker && <UnreadActionIndicator reportActionID={props.action.reportActionID} />} | ||
| <MiniReportActionContextMenu | ||
| reportID={props.report.reportID} | ||
| reportAction={props.action} | ||
| isArchivedRoom={ReportUtils.isArchivedRoom(originalReport)} | ||
| reportActionID={props.action.reportActionID} | ||
| originalReportID={originalReportID} | ||
| isArchivedRoom={ReportUtils.isArchivedRoom(props.report)} | ||
|
Contributor
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. We are using |
||
| displayAsGroup={props.displayAsGroup} | ||
| isVisible={hovered && !props.draftMessage && !hasErrors} | ||
| draftMessage={props.draftMessage} | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
with thread report, this will lead to empty
reportActionbecause it always get thereportActionsof the parent.Slack report: https://expensify.slack.com/archives/C049HHMV9SM/p1694515263690629
cc @dukenv0307 @tgolen @jjcoffee
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.
@hungvu193 Because the structure of the complete task is wrong. This is stored in the task report but the field means this is the parent report action #24644