Fix: Find solution to low resolution large images on Android, and the Canvas crash #20798
Conversation
src/components/AttachmentCarousel/AttachmentCarouselView/index.js
Outdated
Show resolved
Hide resolved
src/components/AttachmentCarousel/AttachmentCarouselView/index.js
Outdated
Show resolved
Hide resolved
src/components/AttachmentCarousel/AttachmentCarouselView/index.js
Outdated
Show resolved
Hide resolved
src/components/AttachmentCarousel/AttachmentCarouselView/index.js
Outdated
Show resolved
Hide resolved
src/components/AttachmentCarousel/AttachmentCarouselView/index.native.js
Outdated
Show resolved
Hide resolved
|
@Julesssss most of the code is already working, but still WIP, so i'll try to finish everything up tomorrow, once i got some replies on this slack thread :) |
|
@Julesssss looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
WRONG |
|
🚀 Deployed to staging by https://github.com/Julesssss in version: 1.3.51-0 🚀
|
|
@chrispader could you please take a look at the above issue to confirm? thanks |
|
🚀 Deployed to production by https://github.com/Julesssss in version: 1.3.51-2 🚀
|
|
🚀 Deployed to staging by https://github.com/Julesssss in version: 1.3.52-0 🚀
|
|
🚀 Deployed to staging by https://github.com/Julesssss in version: 1.3.52-0 🚀
|
| const initialPage = _.findIndex(attachments, (a) => a.source === source); | ||
| if (initialPage === -1) { | ||
| Navigation.dismissModal(); | ||
| return { | ||
| attachments: [], | ||
| initialPage: 0, | ||
| initialItem: undefined, | ||
| initialActiveSource: null, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Previously, an error was thrown that displayed a message to the user.
Why have we modified this to potentially deny the client's request without any context?
If we're confident that this error is no longer possible, we might not need to handle the -1 case.
There was a problem hiding this comment.
Actually this seems to have been changed here: https://github.com/Expensify/App/pull/23072/files#r1288820718
There was a problem hiding this comment.
|
|
||
| const {windowWidth, isSmallScreenWidth} = useWindowDimensions(); | ||
|
|
||
| const {attachments, initialPage, initialActiveSource, initialItem} = useMemo(() => extractAttachmentsFromReport(report, reportActions, source), [report, reportActions, source]); |
There was a problem hiding this comment.
Previously, when the component was class-based, this operation was executed only once. However, it now appears to be triggered whenever source changes. I understand this can happen when navigating to a different attachment or when new messages are received (i.e., when reportActions change).
Given this, if a report has numerous reportActions, we might be processing and extracting attachments from all actions repeatedly each time we navigate. Is this the intended behavior?
There was a problem hiding this comment.
That's a good question to ask! Can you maybe raise that in the expensify open source channel, so we can all discuss it? I am afraid it doesn't have enough visibility here
There was a problem hiding this comment.
Based on this Slack comment i think we can put this aside for now, as this needs a new feature predesign?
Happy to work on this, once we decided on a final approach.
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.3.52-5 🚀
|
src/components/Attachments/AttachmentView/AttachmentViewPdf/index.native.js
Show resolved
Hide resolved
| file={item.file} | ||
| isAuthTokenRequired={item.isAuthTokenRequired} | ||
| isFocused={activeSource === item.source} | ||
| onPress={() => canUseTouchScreen && setShouldShowArrows(!shouldShowArrows)} |
There was a problem hiding this comment.
I think this change introduced regression here #25796
| const [containerDimensions, setContainerDimensions] = useState({width: 0, height: 0}); | ||
| const [page, setPage] = useState(initialPage); | ||
| const [activeSource, setActiveSource] = useState(initialActiveSource); | ||
| const [isPinchGestureRunning, setIsPinchGestureRunning] = useState(true); |
There was a problem hiding this comment.
Hi,
Because the default value was true, we caused Navigation arrows are not shown on PDF until navigated to an image
More details here #24375 (comment)
| const initialPage = _.findIndex(attachments, (a) => a.source === source); | ||
| if (initialPage === -1) { | ||
| Navigation.dismissModal(); |
There was a problem hiding this comment.
Was this fixed somewhere else or do we still need to fix this?
Details
Fixed Issues
$ #18963
PROPOSAL: GH_LINK_ISSUE(COMMENT)
Tests
On iOS/Android:
Offline tests
None needed.
QA Steps
Same as in "Tests"
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
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Simulator.Screen.Recording.-.14Pro.-.2023-07-19.at.14.05.38.mp4
Android
Uploading Screen_Recording_20230719_132241_Expensify Chat.mov…