Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
2 changes: 2 additions & 0 deletions src/ROUTES.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ export default {
getReportRoute: (reportID) => `r/${reportID}`,
REPORT_WITH_ID_DETAILS_SHARE_CODE: 'r/:reportID/details/shareCode',
getReportShareCodeRoute: (reportID) => `r/${reportID}/details/shareCode`,
REPORT_ATTACHMENTS: 'r/:reportID/attachment',
getReportAttachmentRoute: (reportID, source) => `r/${reportID}/attachment?source=${encodeURI(source)}`,
SELECT_YEAR: 'select-year',
getYearSelectionRoute: (minYear, maxYear, currYear, backTo) => `select-year?min=${minYear}&max=${maxYear}&year=${currYear}&backTo=${backTo}`,

Expand Down
1 change: 1 addition & 0 deletions src/SCREENS.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ export default {
HOME: 'Home',
LOADING: 'Loading',
REPORT: 'Report',
REPORT_ATTACHMENTS: 'ReportAttachments',
NOT_FOUND: 'not-found',
TRANSITION_FROM_OLD_DOT: 'TransitionFromOldDot',
};
3 changes: 3 additions & 0 deletions src/components/AttachmentCarousel/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,9 @@ class AttachmentCarousel extends React.Component {
throw new Error('Attachment not found');
}

// Update the parent modal's state with the source and name from the mapped attachments
this.props.onNavigate(attachments[page]);
Comment thread
kidroca marked this conversation as resolved.
Outdated

return {
page,
attachments,
Expand Down
18 changes: 12 additions & 6 deletions src/components/AttachmentModal.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ const propTypes = {
/** Optional callback to fire when we want to preview an image and approve it for use. */
onConfirm: PropTypes.func,

/** Whether the modal should be open by default */
defaultOpen: PropTypes.bool,

/** Optional callback to fire when we want to do something after modal show. */
onModalShow: PropTypes.func,

Expand All @@ -47,7 +50,7 @@ const propTypes = {
originalFileName: PropTypes.string,

/** A function as a child to pass modal launching methods to */
children: PropTypes.func.isRequired,
children: PropTypes.func,

/** Whether source url requires authentication */
isAuthTokenRequired: PropTypes.bool,
Expand All @@ -69,7 +72,9 @@ const propTypes = {
const defaultProps = {
source: '',
onConfirm: null,
defaultOpen: false,
originalFileName: '',
children: null,
isAuthTokenRequired: false,
allowDownload: false,
headerTitle: null,
Expand All @@ -79,7 +84,7 @@ const defaultProps = {
};

function AttachmentModal(props) {
const [isModalOpen, setIsModalOpen] = useState(false);
const [isModalOpen, setIsModalOpen] = useState(props.defaultOpen);
const [shouldLoadAttachment, setShouldLoadAttachment] = useState(false);
const [isAttachmentInvalid, setIsAttachmentInvalid] = useState(false);
const [isAuthTokenRequired] = useState(props.isAuthTokenRequired);
Expand Down Expand Up @@ -343,10 +348,11 @@ function AttachmentModal(props) {
shouldShowCancelButton={false}
/>

{props.children({
displayFileInModal: validateAndDisplayFileToUpload,
show: openModal,
})}
{props.children &&
props.children({
displayFileInModal: validateAndDisplayFileToUpload,
show: openModal,
})}
</>
);
}
Expand Down
45 changes: 19 additions & 26 deletions src/components/HTMLEngineProvider/HTMLRenderers/ImageRenderer.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import React from 'react';
import Navigation from '../../../libs/Navigation/Navigation';
import htmlRendererPropTypes from './htmlRendererPropTypes';
import AttachmentModal from '../../AttachmentModal';
import styles from '../../../styles/styles';
import ThumbnailImage from '../../ThumbnailImage';
import PressableWithoutFocus from '../../Pressable/PressableWithoutFocus';
Expand All @@ -9,6 +9,7 @@ import {ShowContextMenuContext, showContextMenuForReport} from '../../ShowContex
import tryResolveUrlFromApiRoot from '../../../libs/tryResolveUrlFromApiRoot';
import * as ReportUtils from '../../../libs/ReportUtils';
import withLocalize, {withLocalizePropTypes} from '../../withLocalize';
import ROUTES from '../../../ROUTES';

const propTypes = {...htmlRendererPropTypes, ...withLocalizePropTypes};

Expand All @@ -33,7 +34,6 @@ function ImageRenderer(props) {
// control and thus require no authToken to verify access.
//
const isAttachment = Boolean(htmlAttribs[CONST.ATTACHMENT_SOURCE_ATTRIBUTE]);
const originalFileName = htmlAttribs['data-name'];

// Files created/uploaded/hosted by App should resolve from API ROOT. Other URLs aren't modified
const previewSource = tryResolveUrlFromApiRoot(htmlAttribs.src);
Expand All @@ -54,31 +54,24 @@ function ImageRenderer(props) {
) : (
<ShowContextMenuContext.Consumer>
{({anchor, report, action, checkIfContextMenuActive}) => (
<AttachmentModal
allowDownload
report={report}
source={source}
isAuthTokenRequired={isAttachment}
originalFileName={originalFileName}
<PressableWithoutFocus
style={[styles.noOutline]}
onPress={() => {
const route = ROUTES.getReportAttachmentRoute(report.reportID, source);
Navigation.navigate(route);
Comment on lines 60 to 61

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This can be replaced by a call to useNavigation and then navigation.navigate('Report_Attachment', {.reportID, source })

Everywhere else we use Navigation.navigate and ROUTES so I've opted for the same approach here, but this allows deep linking and link sharing and this feature might be unwanted

}}
onLongPress={(event) => showContextMenuForReport(event, anchor, report.reportID, action, checkIfContextMenuActive, ReportUtils.isArchivedRoom(report))}
accessibilityRole="imagebutton"
accessibilityLabel={props.translate('accessibilityHints.viewAttachment')}
>
{({show}) => (
<PressableWithoutFocus
style={[styles.noOutline]}
onPress={show}
onLongPress={(event) => showContextMenuForReport(event, anchor, report.reportID, action, checkIfContextMenuActive, ReportUtils.isArchivedRoom(report))}
accessibilityRole="imagebutton"
accessibilityLabel={props.translate('accessibilityHints.viewAttachment')}
>
<ThumbnailImage
previewSourceURL={previewSource}
style={styles.webViewStyles.tagStyles.img}
isAuthTokenRequired={isAttachment}
imageWidth={imageWidth}
imageHeight={imageHeight}
/>
</PressableWithoutFocus>
)}
</AttachmentModal>
<ThumbnailImage
previewSourceURL={previewSource}
style={styles.webViewStyles.tagStyles.img}
isAuthTokenRequired={isAttachment}
imageWidth={imageWidth}
imageHeight={imageHeight}
/>
</PressableWithoutFocus>
)}
</ShowContextMenuContext.Consumer>
);
Expand Down
12 changes: 12 additions & 0 deletions src/libs/Navigation/AppNavigator/AuthScreens.js
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,18 @@ class AuthScreens extends React.Component {
return ConciergePage;
}}
/>
<RootStack.Screen
name={SCREENS.REPORT_ATTACHMENTS}
options={{
headerShown: false,
presentation: 'transparentModal',
}}
getComponent={() => {
const ReportAttachments = require('../../../pages/home/report/ReportAttachments').default;
return ReportAttachments;
}}
listeners={modalScreenListeners}
/>
Comment on lines +264 to +275

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Utilizing RootStack.Screen proved to be the only viable solution for overlaying the modal over the opened chat. I tested alternative navigators such as CENTRAL_PANE_NAVIGATOR, FULL_SCREEN_NAVIGATOR, and RIGHT_MODAL_NAVIGATOR. However, these alternatives resulted in a blank screen beneath the modal. This undesired outcome was visible for at least half a second during the modal's closing transition.

Sample Behavior When Not Using RootStack

Please refer to the following link for a demonstration of this behavior without the use of RootStack:

Android.Emulator.-.Pixel_2_API_29_5554.2023-06-14.16-28-43.mp4

Sample Behavior When Using RootStack

For comparison, the following link shows the preferred outcome achieved when utilizing RootStack:

Android.Emulator.-.Pixel_2_API_29_5554.2023-06-14.16-31-27.mp4

<RootStack.Screen
name={NAVIGATORS.FULL_SCREEN_NAVIGATOR}
options={defaultScreenOptions}
Expand Down
30 changes: 18 additions & 12 deletions src/libs/Navigation/Navigation.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import navigationRef from './navigationRef';
import NAVIGATORS from '../../NAVIGATORS';
import originalGetTopmostReportId from './getTopmostReportId';
import getStateFromPath from './getStateFromPath';
import SCREENS from '../../SCREENS';

let resolveNavigationIsReadyPromise;
const navigationIsReadyPromise = new Promise((resolve) => {
Expand Down Expand Up @@ -127,19 +128,24 @@ function dismissModal(targetReportID) {
}
const rootState = navigationRef.getRootState();
const lastRoute = _.last(rootState.routes);
if (lastRoute.name === NAVIGATORS.RIGHT_MODAL_NAVIGATOR || lastRoute.name === NAVIGATORS.FULL_SCREEN_NAVIGATOR) {
// if we are not in the target report, we need to navigate to it after dismissing the modal
if (targetReportID && targetReportID !== getTopmostReportId(rootState)) {
const state = getStateFromPath(ROUTES.getReportRoute(targetReportID));

const action = getActionFromState(state, linkingConfig.config);
action.type = 'REPLACE';
navigationRef.current.dispatch(action);
} else {
navigationRef.current.dispatch({...StackActions.pop(), target: rootState.key});
switch (lastRoute.name) {
case NAVIGATORS.RIGHT_MODAL_NAVIGATOR:
case NAVIGATORS.FULL_SCREEN_NAVIGATOR:
case SCREENS.REPORT_ATTACHMENTS:
// if we are not in the target report, we need to navigate to it after dismissing the modal
if (targetReportID && targetReportID !== getTopmostReportId(rootState)) {
const state = getStateFromPath(ROUTES.getReportRoute(targetReportID));

const action = getActionFromState(state, linkingConfig.config);
action.type = 'REPLACE';
navigationRef.current.dispatch(action);
} else {
navigationRef.current.dispatch({...StackActions.pop(), target: rootState.key});
}
break;
default: {
Log.hmmm('[Navigation] dismissModal failed because there is no modal stack to dismiss');
Comment on lines +131 to +147

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nicely done!

}
} else {
Log.hmmm('[Navigation] dismissModal failed because there is no modal stack to dismiss');
}
}

Expand Down
1 change: 1 addition & 0 deletions src/libs/Navigation/linkingConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ export default {
UnlinkLogin: ROUTES.UNLINK_LOGIN,
[SCREENS.TRANSITION_FROM_OLD_DOT]: ROUTES.TRANSITION_FROM_OLD_DOT,
Concierge: ROUTES.CONCIERGE,
[SCREENS.REPORT_ATTACHMENTS]: ROUTES.REPORT_ATTACHMENTS,

// Sidebar
[SCREENS.HOME]: {
Expand Down
40 changes: 40 additions & 0 deletions src/pages/home/report/ReportAttachments.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import React from 'react';
import _ from 'underscore';
import PropTypes from 'prop-types';
import AttachmentModal from '../../../components/AttachmentModal';
import Navigation from '../../../libs/Navigation/Navigation';
import * as ReportUtils from '../../../libs/ReportUtils';

const propTypes = {
/** Navigation route context info provided by react navigation */
route: PropTypes.shape({
/** Route specific parameters used on this screen */
params: PropTypes.shape({
/** The report ID which the attachment is associated with */
reportID: PropTypes.string.isRequired,
/** The uri encoded source of the attachment */
source: PropTypes.string.isRequired,
}).isRequired,
}).isRequired,
};

function ReportAttachments(props) {
const reportID = _.get(props, ['route', 'params', 'reportID']);
const report = ReportUtils.getReport(reportID);
const source = decodeURI(_.get(props, ['route', 'params', 'source']));
Comment on lines +22 to +24

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

NAB: these can all be used inline instead of declared separately

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The source can be a number (viewing assets in native) but the decoded uri is always a string which makes the image fail to load. We needed to preserve the type of the passed source. (Coming from #31138)


return (
<AttachmentModal
allowDownload
defaultOpen
report={report}
source={source}
onModalHide={() => Navigation.dismissModal(reportID)}
/>
);
}

ReportAttachments.propTypes = propTypes;
ReportAttachments.displayName = 'ReportAttachments';

export default ReportAttachments;