Hide flagged attachment in attachment carousel #24564
Conversation
Reviewer Checklist
|
There was a problem hiding this comment.
cc @bernhardoj, left some comments about little bug on native/mWeb
| const {translate} = useLocalize(); | ||
| const [isHidden, setIsHidden] = useState(item.isHidden); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Can we just remove this hook? it's causing this little bug on native/ mWeb Chrome. The attachment is getting hidden before the navigation
CleanShot.2023-08-16.at.13.22.46-converted.mp4
There was a problem hiding this comment.
Open the flagged attachment and try to swipe to the next attachment,then cancel the gesture and you can notice that the attachment is hidden.
There was a problem hiding this comment.
I think we either keep the attachment visible until the next re-render or keep the attachment visible and implement the hide button similar to comments
There was a problem hiding this comment.
I tested moving the logic to componentWillUnmount, it fixes this bug, but it introduces another bug, sometimes the attachment gets hidden after the next preview. I think that's because updatating state doesn’t works in componentWillUnmount !?
There was a problem hiding this comment.
We actually have the viewability config here to update the focus state only when the attachment is 95% visible
App/src/components/Attachments/AttachmentCarousel/index.js
Lines 20 to 24 in 7f13cdd
but somehow this didn't work correctly.
If this works correctly, I think the hook is fine even though it will still revert to hidden when we swipe 95%.
I tested moving the logic to componentWillUnmount
I don't think the CarouselItem is unmounted when we swipe it, except the attachment is out of the flat list window size.
I think that's because updatating state doesn’t works in componentWillUnmount
if the component is unmounted, there is no use to update the state because the state is also gone
I think we either keep the attachment visible until the next re-render or keep the attachment visible
I think this is the best choice for now. If someone later reported a bug to hide the attachment back when navigating to another attachment, then we can reintroduce the hook but also fix the viewability issue of the flatlist. What do you think?
There was a problem hiding this comment.
@shawnborton Ah, okay. Which one do you prefer?
1 (button full width)
2 (button in center)
(button horizontal and vertical margin is 16)
@fedirjh sure, I will test the zooming later
There was a problem hiding this comment.
@shawnborton any preference on the button view above? 👆
There was a problem hiding this comment.
I think the Centered button makes sense, It aligns with the hide button :
There was a problem hiding this comment.
Great, thanks for the confirmation.
@fedirjh there is an issue with the image zooming, pdf is fine, will try to fix it.
| <Text | ||
| style={styles.buttonSmallText} | ||
| selectable={false} | ||
| dataSet={{[CONST.SELECTION_SCRAPER_HIDDEN_ELEMENT]: true}} |
There was a problem hiding this comment.
Added some comments and this dataSet that is also set in the report action item.
App/src/pages/home/report/ReportActionItem.js
Line 358 in c435734
|
@fedirjh I'm currently stuck on the zooming issue (on native). Below is the explanation of the issue. When we zoom an image attachment (that is flagged), the zoomed image won't fit 100% on the container. Please see the video below: Screen.Recording.2023-08-23.at.19.10.46.movCompare it with the image attachment without the Hide message button: Screen.Recording.2023-08-23.at.19.12.01.movThis happens because the The canvas height is taken from the carousel View App/src/components/Attachments/AttachmentCarousel/index.native.js Lines 106 to 110 in 9a0dd42 and it's required before rendering the pager (carousel) App/src/components/Attachments/AttachmentCarousel/index.native.js Lines 133 to 134 in 9a0dd42 This issue actually also happens on an attachment modal when we try to upload a new attachment. So, should we ignore it? or we make the button absolute and without bg? 
|
|
@fedirjh any thoughts on above? |
|
@fedirjh bump ^ |
Hey @bernhardoj, does it only affect flagged attachments? If that's the case, I believe this might not be a bug. The height differs when the hide button is present, hence it's reasonable for the flagged attachment to have a distinct zoom ratio. Therefore, my recommendation is that we take no action. However, I believe there is another bug that needs to be addressed. When we preview a flagged attachment in the report view and then open it in the carousel view, the hidden message is displayed. My expectation is to see the flagged attachment instead. CleanShot.2023-08-28.at.08.52.58.mp4 |
yes
I wouldn't say it's a bug. When we press the attachment, we navigate to the attachment carousel. Handling the case where the user already revealed the attachment on chat would require some changes that I think are not very clean. Here is what we would need to do:
-r/${reportID}/attachment?source=${encodeURI(source)}
+r/${reportID}/attachment?source=${encodeURI(source)}&isRevealed={true/false}
EDIT: we would also need to pass a context to the ImageRenderer to tell whether the current attachment is revealed or not |
I couldn’t reproduce on main, and I am not consistently reproducing it on this branch as well , seems there is something with my emulator, it's iOS 16.4 , I will test on another emulator. |
Screenshots/VideosWebCleanShot.2023-09-05.at.18.14.41.mp4Mobile Web - ChromeCleanShot.2023-09-06.at.11.07.58.mp4Mobile Web - SafariCleanShot.2023-09-06.at.15.40.04.mp4DesktopCleanShot.2023-09-05.at.18.18.09.mp4iOSCleanShot.2023-09-05.at.18.25.49.mp4AndroidCleanShot.2023-09-06.at.10.46.45-converted.mp4 |
| const contextValue = useMemo( | ||
| () => ({ | ||
| isAttachmentHidden, | ||
| updateIsAttachmentHidden: (reportActionID, value) => setIsAttachmentHidden({...isAttachmentHidden, [reportActionID]: value}), |
There was a problem hiding this comment.
| updateIsAttachmentHidden: (reportActionID, value) => setIsAttachmentHidden({...isAttachmentHidden, [reportActionID]: value}), | |
| updateIsAttachmentHidden: (reportActionID, value) => setIsAttachmentHidden((state) => ({...state, [reportActionID]: value})), |
@bernhardoj Why do we use useMemo ?
There was a problem hiding this comment.
To prevent unnecessary rerender. If we pass an object without memo to the context value, the object will always be recreated which triggers the useEffect we use inside ReportActionItem.
There was a problem hiding this comment.
cc @bernhardoj I have some concerns about the performance impact and I left some comments about possible improvements.
| 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.
Can't we create a simple hook, useHiddenAttachements(reportActionID) that returns a boolean, that will simplify the logic and make it more readable?
There was a problem hiding this comment.
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 isAttachmentHidden and updateIsAttachmentHidden are already available through ReportAttachmentsContext and it's still readable. (Actually my honest reason is I don't like creating a new file 😂)
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.
The problem is that any update to isAttachmentHidden will cause a re-render on all mounted CarouselItem components, even those which are not flagged.
There was a problem hiding this comment.
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 isAttachmentHidden state to get updated when we are inside the carousel.
Also, updates to isAttachmentHidden will re-render the whole app.
| 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.
I think this is not really necessary and it just adds overload and complexity to the code.
The reportActionItem will re-render when isAttachmentHidden is updated, isAttachmentHidden should be used only for the attachments carousel.
Actually, all report action items will re-render and this is not really a performance-wise approach.
I would suggest creating a new function updateHidden instead of using a useEffect hook :
updateHidden(hiddenState){
setIsHidden(hiddenState)
updateIsAttachmentHidden(hiddenState)
}
There was a problem hiding this comment.
If we remove the optimization, the app will freeze because of an infinite amount of set state calls.
In the useEffect, we depend on updateIsAttachmentHidden, and calling updateIsAttachmentHidden will update the isAttachmentHidden state which means updateIsAttachmentHidden instance is recreated which then triggers the useEffect again, and so on.
I already tried using your suggestion before but it suffers from the same case.
There was a problem hiding this comment.
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.
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.
@fedirjh can I resolve this one?
There was a problem hiding this comment.
setIsHidden is also being used in this useEffect
App/src/pages/home/report/ReportActionItem.js
Lines 182 to 200 in b11bddc
If we create a new function (let's call it updateHiddenState), we must wrap it in a callback.
useCallback((isHiddenValue) => {
setIsHiddenState(isHiddenValue);
updateHiddenAttachments(actionID, isHiddenValie);
}, [updateHiddenAttachments])
Then the above useEffect will now depend on the new function
[..., updateHiddenState]
Each time hiddenAttachments context is updated, the useEffect will be triggered which will update the state infinitely.
I'm thinking that if hiddenAttachments is updated, the updateHiddenAttachemnts shouldn't be recreated. I will try that and if it works, I will create the new function.
There was a problem hiding this comment.
setIsHiddenis also being used in this useEffect
Hmmm, I wonder why we need to include updateHiddenState in that hook. If a user intends to view the attachment in fullscreen, they should first reveal it.
It would be better to pass updateHiddenState exclusively to the onPress handler of the hide/reveal button. This approach eliminates unnecessary initialization, which, in turn, avoids triggering superfluous updates.
This results in the following evaluation:
- If it's hidden and not revealed, take no action.
- If it's hidden and revealed, initialize and update the context. This ensures that we minimize unnecessary updates, with updates contingent on user interactions.
Currently, the code operates as follows:
All attachments are added to the context, and their values are set to either true or false based on their hidden status.
I'm thinking that if hiddenAttachments is updated, the updateHiddenAttachemnts shouldn't be recreated.
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.
Ah, you're right. I already handle the case where the state is undefined here.
Maybe using ref to store the state would help achieve that.
Yep, this is what I recently thought and tested. The hiddenAttachments will be a ref instead of a state. Then, we will expose a new function called isAttachmentHidden(reportActionID) through the context, so CarouselItem can get the ref value. This way, updating hiddenAttachments won't re-render the whole app. We don't need the reactivity from the state, so I think it's fine to use ref. What do you think?
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.
@bernhardoj That makes more sense. Let's proceed with this approach.
There was a problem hiding this comment.
@fedirjh Great, pushed the new updated.
There was a problem hiding this comment.
Reviewer Checklist
- I have verified the author checklist is complete (all boxes are checked off).
- I verified the correct issue is linked in the
### Fixed Issuessection above - I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the
Testssection - I verified the steps for Staging and/or Production testing are in the
QA stepssection - I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
- I verified the steps for local testing are in the
- I checked that screenshots or videos are included for tests on all platforms
- I included screenshots or videos for tests on all platforms
- I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
- If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
- I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e.
toggleReportand notonIconClick). - I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g.
myBool && <MyComponent />. - I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to
src/languages/*files and using the translation method - I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the
Waiting for Copylabel for a copy review on the original GH to get the correct copy. - I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in
STYLE.md) were followed
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e.
- If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
- I verified that this PR follows the guidelines as stated in the Review Guidelines
- I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like
Avatar, I verified the components usingAvatarhave been tested & I retested again) - I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
- I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
- If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a
/** comment above it */ - The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to
thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor) - Any internal methods bound to
thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick) - All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
- If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
- If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e.
StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
- If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
- If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like
Avataris modified, I verified thatAvataris working as expected in all cases) - If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
- If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
- If a new page is added, I verified it's using the
ScrollViewcomponent to make it scrollable when more elements are added to the page. - If the
mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps. - I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.
Screenshots/Videos
Web
CleanShot.2023-09-05.at.18.14.41.mp4
Mobile Web - Chrome
CleanShot.2023-09-06.at.11.07.58.mp4
Mobile Web - Safari
CleanShot.2023-09-06.at.15.40.04.mp4
Desktop
CleanShot.2023-09-05.at.18.18.09.mp4
iOS
CleanShot.2023-09-05.at.18.25.49.mp4
Android
CleanShot.2023-09-06.at.10.46.45-converted.mp4
| const isAttachmentHiddenValue = isAttachmentHidden(item.reportActionID); | ||
| return isAttachmentHiddenValue === undefined ? item.hasBeenFlagged : isAttachmentHiddenValue; |
There was a problem hiding this comment.
NAB but I'd prefer isAttachmentHidden
There was a problem hiding this comment.
It will conflict with the isAttachmentHidden function, so I need to append Value to it
There was a problem hiding this comment.
I think it could be simplified using the null-coalescing operator ??
const [isHidden, setIsHidden] = useState(isAttachmentHidden(item.reportActionID) ?? item.hasBeenFlagged);There was a problem hiding this comment.
That's a good idea, but the lint forbids null coalescing operators
There was a problem hiding this comment.
Maybe we can pass that rule ?
FYI, nullish coalescing will be supported as part of the TypeScript migration. You can find more details in the following link: TS_STYLE.md
There was a problem hiding this comment.
Let's go for it, I think that improves readability considerably and is valid given we're supporting it soon.
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/amyevans in version: 1.3.69-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.3.69-2 🚀
|
|
🚀 Deployed to staging by https://github.com/amyevans in version: 1.3.70-0 🚀
|
|
🚀 Deployed to production by https://github.com/thienlnam in version: 1.3.70-8 🚀
|
Details
When we flagged an attachment as intimidation/bullying, the attachment becomes hidden in the chat screen. However, we didn't do the same when the user is viewing the attachment in the carousel. This PR hides the attachment in the carousel too.
Fixed Issues
$ #22915
PROPOSAL: #22915 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
Screen.Recording.2023-08-28.at.17.39.13.mov
Mobile Web - Chrome
Screen.Recording.2023-08-28.at.17.44.24.mov
Mobile Web - Safari
Screen.Recording.2023-08-28.at.17.43.33.mov
Desktop
Screen.Recording.2023-08-28.at.17.46.17.mov
iOS
Screen.Recording.2023-08-28.at.17.41.19.mov
Android
Screen.Recording.2023-08-28.at.17.42.18.mov