Fix Opened offline attachment directed to conversation page on online #49832
+60
−63
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tachment replaced / deleted
… index is similar
…x/48173-dismiss-when-no-attachment-with-same-page-index
…x/48173-dismiss-when-no-attachment-with-same-page-index
Open
6 tasks
s77rt
suggested changes
Sep 27, 2024
Co-authored-by: Abdelhafidh Belalia <[email protected]>
s77rt
suggested changes
Sep 28, 2024
| let initialPage = targetAttachments.findIndex(compareImage); | ||
| const prevInitialPage = attachments.findIndex(compareImage); | ||
|
|
||
| // If no matching attachment is found in targetAttachments but found in attachments, update initialPage |
There was a problem hiding this comment.
This comment is explaining the what not the why. Let's remove it
|
|
||
| // Dismiss the modal when deleting an attachment during its display in preview. | ||
| if (initialPage === -1 && attachments.find(compareImage)) { | ||
| // If no matching attachment with the same index, dismiss the modal |
Comment on lines
92
to
93
| let initialPage = targetAttachments.findIndex(compareImage); | ||
| const prevInitialPage = attachments.findIndex(compareImage); |
There was a problem hiding this comment.
Suggested change
| let initialPage = targetAttachments.findIndex(compareImage); | |
| const prevInitialPage = attachments.findIndex(compareImage); | |
| let attachmentIndex = targetAttachments.findIndex(compareImage); | |
| const prevAttachmentIndex = attachments.findIndex(compareImage); |
I feel that this naming is more correct. findIndex returns an index and not a page. The index is used as the page id.
| const prevInitialPage = attachments.findIndex(compareImage); | ||
|
|
||
| // If no matching attachment is found in targetAttachments but found in attachments, update initialPage | ||
| if (initialPage === -1 && prevInitialPage !== -1 && targetAttachments[prevInitialPage]) { |
There was a problem hiding this comment.
Can we avoid comparing with -1 and instead just access the attachment array directly
if (!targetAttachments[attachmentIndex] && targetAttachments[prevAttachmentIndex])…x/48173-dismiss-when-no-attachment-with-same-page-index
s77rt
suggested changes
Sep 29, 2024
Comment on lines
43
to
44
| let attachmentIndex = targetAttachments.findIndex(compareImage); | ||
| const prevAttachmentIndex = attachments.findIndex(compareImage); |
There was a problem hiding this comment.
NAB. Another naming clarity. targetAttachments is confusing.
- targetAttachments -> newAttachments
- attachmentIndex -> newIndex
- prevAttachmentIndex -> index
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid.movAndroid: mWeb Chromemweb-chrome.moviOS: Nativeios.moviOS: mWeb Safarimweb-safari.movMacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov |
|
@wildan-m Can you check why on native the attachment changes after getting online? ios.movandroid.mov |
…x/48173-dismiss-when-no-attachment-with-same-page-index
that's pretty weird. @s77rt do you know how to make metro keep logging the log after re-connect? |
|
@wildan-m Try use Xcode or Android Studio.
|
s77rt
reviewed
Sep 30, 2024
Comment on lines
92
to
93
| let attachmentIndex = targetAttachments.findIndex(compareImage); | ||
| const prevAttachmentIndex = attachments.findIndex(compareImage); |
There was a problem hiding this comment.
Can you do same refactor on the names on this file too
|
@wildan-m Can you clarify how |
|
@s77rt it appears that's an upstream issue, I'll try to reproduce it with simple example. If that's an upstream issue, should we fix it with this PR?
It only remounts if we encounter this condition, I think that will not hurt the performance that much if (!newAttachments[newIndex] && newAttachments[index]) { |
…x/48173-dismiss-when-no-attachment-with-same-page-index
|
@s77rt I've created this simple reproduction step, even without scrolling from reanimated, the pager view itself can't properly work if we replace the pager items without re-mounting. Kapture.2024-10-01.at.10.31.56.mp4I think we don't need to report the issue as there are similar issues open. Minimum reproducible codeimport React from 'react';
import { useState } from 'react';
import { Button, SafeAreaView, Text, View, StyleSheet } from 'react-native';
import PagerView from 'react-native-pager-view';
const ids = ['1', '2', '3'];
function getNewArray() {
const newArray = [...ids];
newArray[2] = Math.random().toString(36).substring(7); // Generate a random string
return newArray;
}
export const BasicPagerViewExample = () => {
const [pagesContent, setPagesContent] = useState<string[]>(getNewArray());
const [page, setPage] = useState<number>(2);
console.log(pagesContent);
return (
<SafeAreaView style={{ flex: 1 }}>
<Text style={{ textAlign: 'center' }}>{pagesContent.length}</Text>
<PagerView orientation="horizontal" style={{ flex: 1, backgroundColor: 'yellow' }} initialPage={page}
onPageSelected={(e) => {
console.log('onPageSelected ~ e.nativeEvent.position:', e.nativeEvent.position);
}}
>
{pagesContent.map((content) => (
<View key={content} style={{ flex: 1,backgroundColor: 'green' }}>
<View style={{ flex: 1, justifyContent: 'center', alignItems: 'center' , backgroundColor:'red'}}>
<Text style={{ fontSize: 30 }}>{content}</Text>
</View>
</View>
))}
</PagerView>
<Button
title={'Replace third page content'}
onPress={() => {
setPagesContent(getNewArray());
setPage(2);
}}
/>
</SafeAreaView>
);
};
export default BasicPagerViewExample
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Details
Fix offline attachment opening directly to online conversation page. New dismissal behavior confirmed here: #48173 (comment)
Fixed Issues
$ #48173
PROPOSAL: #48173 (comment) (Alternative 3)
Tests
Offline tests
Same as test
QA Steps
Same as test
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.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
Android: Native
Kapture.2024-09-27.at.14.02.17.mp4
Android: mWeb Chrome
Kapture.2024-09-27.at.14.05.10.mp4
iOS: Native
Kapture.2024-09-27.at.11.02.28.mp4
iOS: mWeb Safari
Kapture.2024-09-27.at.13.31.57.mp4
MacOS: Chrome / Safari
Kapture.2024-09-27.at.10.41.52.mp4
MacOS: Desktop
Kapture.2024-09-27.at.14.06.18.mp4