-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Hide flagged attachment in attachment carousel #24564
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 25 commits
f53d3dc
6d0190c
104f0f4
7dfcf17
93fe8c3
0fd41e9
6b6f267
95045a5
1734b16
83c629e
d5b8130
4dbf950
e7956b3
8863e79
dce6721
fc337d6
4202190
4d41062
8d22119
e850d52
3f396d6
0b586f4
619af69
f151ae0
c315df5
fe07eba
0315c87
7244223
31385c3
b9ef612
ab4d5f3
9a0755c
4df91a2
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 |
---|---|---|
@@ -0,0 +1,117 @@ | ||
import React, {useContext, useState} from 'react'; | ||
import {View} from 'react-native'; | ||
import PropTypes from 'prop-types'; | ||
import CONST from '../../../CONST'; | ||
import styles from '../../../styles/styles'; | ||
import useLocalize from '../../../hooks/useLocalize'; | ||
import PressableWithoutFeedback from '../../Pressable/PressableWithoutFeedback'; | ||
import Text from '../../Text'; | ||
import Button from '../../Button'; | ||
import AttachmentView from '../AttachmentView'; | ||
import SafeAreaConsumer from '../../SafeAreaConsumer'; | ||
import ReportAttachmentsContext from '../../../pages/home/report/ReportAttachmentsContext'; | ||
|
||
const propTypes = { | ||
/** Attachment required information such as the source and file name */ | ||
item: PropTypes.shape({ | ||
/** Report action ID of the attachment */ | ||
reportActionID: PropTypes.string, | ||
|
||
/** Whether source URL requires authentication */ | ||
isAuthTokenRequired: PropTypes.bool, | ||
|
||
/** The source (URL) of the attachment */ | ||
source: PropTypes.string, | ||
|
||
/** File additional information of the attachment */ | ||
file: PropTypes.shape({ | ||
/** File name of the attachment */ | ||
name: PropTypes.string, | ||
}), | ||
|
||
/** Whether the attachment has been flagged */ | ||
hasBeenFlagged: PropTypes.bool, | ||
}).isRequired, | ||
|
||
/** Whether the attachment is currently being viewed in the carousel */ | ||
isFocused: PropTypes.bool.isRequired, | ||
|
||
/** onPress callback */ | ||
onPress: PropTypes.func, | ||
}; | ||
|
||
const defaultProps = { | ||
onPress: undefined, | ||
}; | ||
|
||
function CarouselItem({item, isFocused, onPress}) { | ||
const {translate} = useLocalize(); | ||
const {isAttachmentHidden} = useContext(ReportAttachmentsContext); | ||
const [isHidden, setIsHidden] = useState(() => { | ||
const isAttachmentHiddenValue = isAttachmentHidden[item.reportActionID]; | ||
return isAttachmentHiddenValue === undefined ? item.hasBeenFlagged : isAttachmentHiddenValue; | ||
}); | ||
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't we create a simple hook, 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. Definitely can, but I decided to not create it because it doesn't solve any actual problem. It reduces the number of characters we type but both Before: const {isAttachmentHidden} = useContext(ReportAttachmentsContext);
const [isHidden, setIsHidden] = useState(() => {
const isAttachmentHiddenValue = isAttachmentHidden[item.reportActionID];
return isAttachmentHiddenValue === undefined ? item.hasBeenFlagged : isAttachmentHiddenValue;
}); After: const isAttachmentHidden = useHiddenAttachments(item.reportActionID);
const [isHidden, setIsHidden] = useState(() => isAttachmentHidden === undefined ? item.hasBeenFlagged : isAttachmentHidden); Let me know if it makes sense to not create the hook. 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 problem is that any update to 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. That will be the same case with using hook. Using hook will still require us to access the context. Btw, it's not possible for the Also, updates to |
||
|
||
const renderButton = (style) => ( | ||
<Button | ||
small | ||
style={style} | ||
onPress={() => setIsHidden(!isHidden)} | ||
> | ||
<Text | ||
style={styles.buttonSmallText} | ||
selectable={false} | ||
dataSet={{[CONST.SELECTION_SCRAPER_HIDDEN_ELEMENT]: true}} | ||
> | ||
{isHidden ? translate('moderation.revealMessage') : translate('moderation.hideMessage')} | ||
</Text> | ||
</Button> | ||
); | ||
|
||
if (isHidden) { | ||
const children = ( | ||
<> | ||
<Text style={[styles.textLabelSupporting, styles.textAlignCenter, styles.lh20]}>{translate('moderation.flaggedContent')}</Text> | ||
{renderButton([styles.mt2])} | ||
</> | ||
); | ||
return onPress ? ( | ||
<PressableWithoutFeedback | ||
style={[styles.attachmentRevealButtonContainer]} | ||
onPress={onPress} | ||
accessibilityRole={CONST.ACCESSIBILITY_ROLE.IMAGEBUTTON} | ||
accessibilityLabel={item.file.name || translate('attachmentView.unknownFilename')} | ||
> | ||
{children} | ||
</PressableWithoutFeedback> | ||
) : ( | ||
<View style={[styles.attachmentRevealButtonContainer]}>{children}</View> | ||
); | ||
} | ||
|
||
return ( | ||
<View style={[styles.flex1]}> | ||
<View style={[styles.flex1]}> | ||
<AttachmentView | ||
source={item.source} | ||
file={item.file} | ||
isAuthTokenRequired={item.isAuthTokenRequired} | ||
isFocused={isFocused} | ||
onPress={onPress} | ||
isUsedInCarousel | ||
/> | ||
</View> | ||
|
||
{item.hasBeenFlagged && ( | ||
<SafeAreaConsumer> | ||
{({safeAreaPaddingBottomStyle}) => <View style={[styles.appBG, safeAreaPaddingBottomStyle]}>{renderButton([styles.m4, styles.alignSelfCenter])}</View>} | ||
</SafeAreaConsumer> | ||
)} | ||
</View> | ||
); | ||
} | ||
|
||
CarouselItem.propTypes = propTypes; | ||
CarouselItem.defaultProps = defaultProps; | ||
|
||
export default CarouselItem; |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -67,6 +67,7 @@ import * as BankAccounts from '../../../libs/actions/BankAccounts'; | |||||||||||||||||||||||||||||||||||||||
import usePrevious from '../../../hooks/usePrevious'; | ||||||||||||||||||||||||||||||||||||||||
import ReportScreenContext from '../ReportScreenContext'; | ||||||||||||||||||||||||||||||||||||||||
import Permissions from '../../../libs/Permissions'; | ||||||||||||||||||||||||||||||||||||||||
import ReportAttachmentsContext from './ReportAttachmentsContext'; | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
const propTypes = { | ||||||||||||||||||||||||||||||||||||||||
...windowDimensionsPropTypes, | ||||||||||||||||||||||||||||||||||||||||
|
@@ -129,13 +130,23 @@ function ReportActionItem(props) { | |||||||||||||||||||||||||||||||||||||||
const [isHidden, setIsHidden] = useState(false); | ||||||||||||||||||||||||||||||||||||||||
const [moderationDecision, setModerationDecision] = useState(CONST.MODERATION.MODERATOR_DECISION_APPROVED); | ||||||||||||||||||||||||||||||||||||||||
const {reactionListRef} = useContext(ReportScreenContext); | ||||||||||||||||||||||||||||||||||||||||
const {isAttachmentHidden, updateIsAttachmentHidden} = useContext(ReportAttachmentsContext); | ||||||||||||||||||||||||||||||||||||||||
const textInputRef = useRef(); | ||||||||||||||||||||||||||||||||||||||||
const popoverAnchorRef = useRef(); | ||||||||||||||||||||||||||||||||||||||||
const downloadedPreviews = useRef([]); | ||||||||||||||||||||||||||||||||||||||||
const prevDraftMessage = usePrevious(props.draftMessage); | ||||||||||||||||||||||||||||||||||||||||
const originalReportID = ReportUtils.getOriginalReportID(props.report.reportID, props.action); | ||||||||||||||||||||||||||||||||||||||||
const originalReport = props.report.reportID === originalReportID ? props.report : ReportUtils.getReport(originalReportID); | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
useEffect(() => { | ||||||||||||||||||||||||||||||||||||||||
const isAttachment = ReportUtils.isReportMessageAttachment(_.last(props.action.message)); | ||||||||||||||||||||||||||||||||||||||||
// To prevent unnecessary re-render, skip updating the state if the value is the same | ||||||||||||||||||||||||||||||||||||||||
if (!isAttachment || Boolean(isAttachmentHidden[props.action.reportActionID]) === isHidden) { | ||||||||||||||||||||||||||||||||||||||||
return; | ||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||
updateIsAttachmentHidden(props.action.reportActionID, isHidden); | ||||||||||||||||||||||||||||||||||||||||
}, [props.action.reportActionID, props.action.message, isHidden, isAttachmentHidden, updateIsAttachmentHidden]); | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
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. I think this is not really necessary and it just adds overload and complexity to the code. The Actually, all I would suggest creating a new function updateHidden(hiddenState){
setIsHidden(hiddenState)
updateIsAttachmentHidden(hiddenState)
}
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. If we remove the optimization, the app will freeze because of an infinite amount of set state calls. In the I already tried using your suggestion before but it suffers from the same case. 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. Actually, I think we can move the optimization up to the context. I will try it and push if it works fine. 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. Actually, I think we can move the optimization up to the context. I will try it and push if it works fine. 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. @fedirjh can I resolve this one? 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.
App/src/pages/home/report/ReportActionItem.js Lines 182 to 200 in b11bddc
If we create a new function (let's call it
Then the above
Each time I'm thinking that if 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.
Hmmm, I wonder why we need to include It would be better to pass This results in the following evaluation:
Currently, the code operates as follows: All attachments are added to the context, and their values are set to either
This would ultimately resolve any unnecessary re-renders. Maybe using ref to store the state would help achieve that. 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, you're right. I already handle the case where the state is undefined here.
Yep, this is what I recently thought and tested. The diff --git a/src/components/Attachments/AttachmentCarousel/CarouselItem.js b/src/components/Attachments/AttachmentCarousel/CarouselItem.js
index b5b1e0352c..538ebac8d7 100644
--- a/src/components/Attachments/AttachmentCarousel/CarouselItem.js
+++ b/src/components/Attachments/AttachmentCarousel/CarouselItem.js
@@ -46,10 +46,10 @@ const defaultProps = {
function CarouselItem({item, isFocused, onPress}) {
const {translate} = useLocalize();
- const {hiddenAttachments} = useContext(ReportAttachmentsContext);
+ const {isAttachmentHidden} = useContext(ReportAttachmentsContext);
const [isHidden, setIsHidden] = useState(() => {
- const isAttachmentHidden = hiddenAttachments[item.reportActionID];
- return isAttachmentHidden === undefined ? item.hasBeenFlagged : isAttachmentHidden;
+ const isAttachmentHiddenValue = isAttachmentHidden(item.reportActionID);
+ return isAttachmentHiddenValue === undefined ? item.hasBeenFlagged : isAttachmentHiddenValue;
});
const renderButton = (style) => (
diff --git a/src/pages/home/report/ReportAttachmentsContext.js b/src/pages/home/report/ReportAttachmentsContext.js
index d763f9389c..cca47b54aa 100644
--- a/src/pages/home/report/ReportAttachmentsContext.js
+++ b/src/pages/home/report/ReportAttachmentsContext.js
@@ -1,4 +1,4 @@
-import React, {useEffect, useMemo, useState} from 'react';
+import React, {useEffect, useMemo, useRef} from 'react';
import PropTypes from 'prop-types';
import useCurrentReportID from '../../../hooks/useCurrentReportID';
@@ -11,26 +11,25 @@ const propTypes = {
function ReportAttachmentsProvider(props) {
const currentReportID = useCurrentReportID();
- const [hiddenAttachments, setHiddenAttachments] = useState({});
+ const hiddenAttachments = useRef({});
useEffect(() => {
// We only want to store the attachment visibility for the current report.
// If the current report ID changes, clear the state.
- setHiddenAttachments({});
+ hiddenAttachments.current = {};
}, [currentReportID]);
const contextValue = useMemo(
() => ({
- hiddenAttachments,
+ isAttachmentHidden: (reportActionID) => hiddenAttachments.current[reportActionID],
updateHiddenAttachments: (reportActionID, value) => {
- // To prevent unnecessary re-render, skip updating the state if the value is the same
- if (Boolean(hiddenAttachments[reportActionID]) === value) {
- return;
- }
- setHiddenAttachments((state) => ({...state, [reportActionID]: value}));
+ hiddenAttachments.current = {
+ ...hiddenAttachments.current,
+ [reportActionID]: value,
+ };
},
}),
- [hiddenAttachments],
+ [],
);
return <ReportAttachmentsContext.Provider value={contextValue}>{props.children}</ReportAttachmentsContext.Provider>;
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. @bernhardoj That makes more sense. Let's proceed with this approach. 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. @fedirjh Great, pushed the new updated.
bernhardoj marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||
useEffect( | ||||||||||||||||||||||||||||||||||||||||
() => () => { | ||||||||||||||||||||||||||||||||||||||||
// ReportActionContextMenu, EmojiPicker and PopoverReactionList are global components, | ||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
import React, {useEffect, useMemo, useState} from 'react'; | ||
import PropTypes from 'prop-types'; | ||
import useCurrentReportID from '../../../hooks/useCurrentReportID'; | ||
|
||
const ReportAttachmentsContext = React.createContext(); | ||
|
||
const propTypes = { | ||
/** Rendered child component */ | ||
children: PropTypes.node.isRequired, | ||
}; | ||
|
||
function ReportAttachmentsProvider(props) { | ||
const currentReportID = useCurrentReportID(); | ||
const [isAttachmentHidden, setIsAttachmentHidden] = useState({}); | ||
bernhardoj marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
useEffect(() => { | ||
// We only want to store the attachment visibility for the current report. | ||
// If the current report ID changes, clear the state. | ||
setIsAttachmentHidden({}); | ||
}, [currentReportID]); | ||
|
||
const contextValue = useMemo( | ||
() => ({ | ||
isAttachmentHidden, | ||
updateIsAttachmentHidden: (reportActionID, value) => setIsAttachmentHidden((state) => ({...state, [reportActionID]: value})), | ||
}), | ||
[isAttachmentHidden], | ||
); | ||
|
||
return <ReportAttachmentsContext.Provider value={contextValue}>{props.children}</ReportAttachmentsContext.Provider>; | ||
} | ||
|
||
ReportAttachmentsProvider.propTypes = propTypes; | ||
ReportAttachmentsProvider.displayName = 'ReportAttachmentsProvider'; | ||
|
||
export default ReportAttachmentsContext; | ||
export {ReportAttachmentsProvider}; |
Uh oh!
There was an error while loading. Please reload this page.