Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Hide flagged attachment in attachment carousel #24564
Hide flagged attachment in attachment carousel #24564
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
There are no files selected for viewing
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.
Can't we create a simple hook,
useHiddenAttachements(reportActionID)
that returns aboolean
, that will simplify the logic and make it more readable?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.
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
andupdateIsAttachmentHidden
are already available through ReportAttachmentsContext and it's still readable. (Actually my honest reason is I don't like creating a new file 😂)Before:
After:
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 comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that any update to
isAttachmentHidden
will cause a re-render on all mountedCarouselItem
components, even those which are not flagged.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.
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.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.
I think this is not really necessary and it just adds overload and complexity to the code.
The
reportActionItem
will re-render whenisAttachmentHidden
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 auseEffect
hook :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.
If we remove the optimization, the app will freeze because of an infinite amount of set state calls.
In the
useEffect
, we depend onupdateIsAttachmentHidden
, and callingupdateIsAttachmentHidden
will update theisAttachmentHidden
state which meansupdateIsAttachmentHidden
instance is recreated which then triggers theuseEffect
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.
Choose a reason for hiding this comment
The 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 comment
The 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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
setIsHidden
is also being used in this useEffectApp/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.Then the above
useEffect
will now depend on the new function[..., updateHiddenState]
Each time
hiddenAttachments
context is updated, theuseEffect
will be triggered which will update the state infinitely.I'm thinking that if
hiddenAttachments
is updated, theupdateHiddenAttachemnts
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theonPress
handler of the hide/reveal button. This approach eliminates unnecessary initialization, which, in turn, avoids triggering superfluous updates.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
true
orfalse
based on their hidden status.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 comment
The 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
hiddenAttachments
will be a ref instead of a state. Then, we will expose a new function calledisAttachmentHidden(reportActionID)
through the context, so CarouselItem can get the ref value. This way, updatinghiddenAttachments
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?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.
@bernhardoj That makes more sense. Let's proceed with this approach.
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.
@fedirjh Great, pushed the new updated.