-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Attachment Carousel Virtual Caching #16973
Attachment Carousel Virtual Caching #16973
Conversation
138a15f
to
27e498b
Compare
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.
Added some notes on the changes
@@ -75,14 +90,28 @@ class AttachmentCarousel extends React.Component { | |||
getAttachment(attachmentItem) { | |||
const source = _.get(attachmentItem, 'source', ''); | |||
const file = _.get(attachmentItem, 'file', {name: ''}); | |||
this.props.onNavigate({source: addEncryptedAuthTokenToURL(source), file}); |
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.
This side effect was moved to the appropriate place - at the updatePage
method - it shouldn't be in a getter
if ((deltaSlide > 0 && this.state.isForwardDisabled) || (deltaSlide < 0 && this.state.isBackDisabled)) { | ||
const nextIndex = this.state.page - deltaSlide; | ||
const nextItem = this.state.attachments[nextIndex]; | ||
|
||
if (!nextItem) { | ||
return; |
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 delta > 0 || < 0
checks are simplified:
We try to access an item - if the computed index returns an item - we use otherwise not
(This also lets us remove isForwardDisabled
and isBackDisabled
from local state)
this.setState(({attachments, page}) => { | ||
const nextIndex = page - deltaSlide; | ||
const {source, file} = this.getAttachment(attachments[nextIndex]); | ||
return { | ||
page: nextIndex, | ||
source, | ||
file, | ||
isBackDisabled: nextIndex === attachments.length - 1, | ||
isForwardDisabled: nextIndex === 0, | ||
}; | ||
}); | ||
// The sliding transition is a bit too much on web, because of the wider and bigger images, | ||
// so we only enable it for mobile | ||
this.scrollRef.current.scrollToIndex({index: nextIndex, animated: this.canUseTouchScreen}); |
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 state update was moved to updatePage
Here we only tell the view to scroll, which in turn would trigger updatePage
and update the state
// Touch screen devices can toggle between showing and hiding the arrows by tapping on the image/container | ||
// Other devices toggle the arrows through hovering (mouse) instead (see render() root element) | ||
if (!this.canUseTouchScreen) { | ||
// eslint-disable-next-line react/jsx-props-no-spreading | ||
return <View {...props} style={style} />; | ||
} | ||
|
||
return ( | ||
<Pressable | ||
// eslint-disable-next-line react/jsx-props-no-spreading | ||
{...props} | ||
onPress={() => this.setState(current => ({shouldShowArrow: !current.shouldShowArrow}))} | ||
style={style} | ||
/> | ||
); |
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.
This is something I picked up while reviewing the onPress
of CarouselActions
I moved it here as it was the only thing left for CarouselActions/index.native.js
Having the Pressable
wrap the FlatList is undesirable because it interferes with swiping - now we're no longer wrapping Attachments (FlatList) with Pressable (CarouselActions)
onCycleThroughAttachments={this.cycleThroughAttachments} | ||
> | ||
<AttachmentView | ||
onPress={() => this.toggleArrowsVisibility(!this.state.shouldShowArrow)} |
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.
Follow up refactor - the onPress
prop seem to have been added to the AttchmentView
and then all the child view that it renders (Image, Pdf, ...) for the arrow functionality
There are no more usages of AttachmentView with onPress
, so we can remove it (as well as the other views like ImageView)
@johnmlee101 @thesahindia One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
That looks neat! Also thanks for the additional changes. I just noticed few things while testing -
|
Which platform - I don't have a problem on iOS, Chrome, Desktop, mobile web. I'll test Android tomorrow
This is because of the snap to alignment feature, I'll look into it
This is from successful virtualization - the image doesn't reload, it's properties aren't changed it's just moved into view. For such a small bug I wouldn't risk adding complex logic to try and fix it |
Large `updateCellsBatchingPeriod` values affect negatively swiping because view is getting added and removed and this causes the list content to shift When we don't set `updateCellsBatchingPeriod` this happens quicker and is less if even noticeable
…rows for touch devices
These changes were necessary to alleviate a bug that happened because image width/height would briefly be set to 0 and there was a noticeable jump while scrolling At some point the issue disappeared on its own, probably because of less re-renders
We can't guarantee the carousel will always be as wide as the window, so it's better to use the containing layout width For example On web/desktop the carousel is not rendered in a fullscreen modal and is slightly smaller than the whole window width
27e498b
to
62678f7
Compare
I've pushed a fix for these 2 items Regarding the issue with resetting zoom
I found out how to rest zoom for images, but not for PDF documents I would prefer to address this in a separate PR as it would require a bit more changes and testing Let's first confirm whether we want to fix this at all ✅ Ready for Review (pulled latest changes from |
Since it's not anything major I am fine with doing nothing here, but let's confirm this with @johnmlee101. @johnmlee101, can you please share your thoughts? |
When I increase the screen size I can see the image list and sometimes the image changes. @kidroca, can we fix this? Screen.Recording.2023-04-15.at.7.51.05.PM.movOther than this it worked perfectly. I didn't face any other issue. |
|
||
componentDidMount() { | ||
this.makeStateWithReports(); | ||
this.state = { |
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 am not sure if it's fine to overwrite the state. Can we use componentDidMount
?
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.
Move this to componentDidMount
The zoom seems fine, I don't think it's much of a "bug" to have it retain the previous state of when you were viewing it |
I don't think there's anything easy to do about it - the app is laggy when it's resized like that |
@johnmlee101, should we create a new GH for this? |
Reviewer Checklist
Screenshots/VideosWebScreen.Recording.2023-04-15.at.6.01.10.PM.movMobile Web - ChromeScreen.Recording.2023-04-18.at.8.10.09.PM.movMobile Web - SafariScreen.Recording.2023-04-15.at.7.48.30.PM.movDesktopScreen.Recording.2023-04-15.at.6.20.08.PM.moviOSScreen.Recording.2023-04-15.at.7.08.18.PM.movAndroidScreen.Recording.2023-04-18.at.8.14.55.PM.mov |
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.
Works well!
cc: @johnmlee101
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.
This worked very well! Thanks for this
@johnmlee101, you missed this comment. |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Ah I see. I think we can create a follow-up issue to see if others have more opinions about it. It does seem like a very tricky thing to implement so not sure if its worth changing. Screen size changes aren't a common occurrence for most users. |
🚀 Deployed to staging by https://github.com/johnmlee101 in version: 1.3.2-0 🚀
|
Posted it here.
Agree! Let's see if there is any simple solution. |
🚀 Deployed to production by https://github.com/mountiny in version: 1.3.2-5 🚀
|
* @returns {JSX.Element} | ||
*/ | ||
renderCell(props) { | ||
const style = [props.style, styles.h100, {width: this.state.containerWidth}]; |
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.
return ( | ||
<View | ||
style={[styles.attachmentModalArrowsContainer]} | ||
style={[styles.attachmentModalArrowsContainer, styles.flex1]} | ||
onLayout={({nativeEvent}) => this.setState({containerWidth: nativeEvent.layout.width + 1})} |
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.
Heads up!! This line caused a regression in #18222. On iOS, the left edge of the PDF is visible on the previous image due to a missing 1px
border calculation . We ended up removing the borderWidth
.
file={this.state.file} | ||
|
||
{this.state.containerWidth > 0 && ( | ||
<FlatList |
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.
Coming from #19484:
While introducing FlatList for carousel, it caused weird behavior when press Arrow Left/Right keys.
Because FlatList wants to scroll horizontally as default and it conflicts with main navigating feature of arrow keys.
To fix this, we prevented default (e.preventDefault()) so that FlatList auto scroll doesn't work.
Details
Add a virtual list to the carousel component that is used to display attachments in the message view.
This allows us to render (and preload) a large number of attachments without having to render them all at once.
Now navigating between attachments is instant because we preload the next attachment in the background.
Fixed Issues
$ #15922
PROPOSAL: #15922 (comment)
Tests
Preparation
Things to check
Offline tests
Offline behavior is same as before:
QA Steps
The following steps are for both STAGING and PRODUCTION environments and all platforms
Preparation
Things to check
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
1.New.Expensify.-.Google.Chrome.2023-04-05.22-18-37.mp4
Screen.Recording.2023-04-06.at.19.17.56.mov
Mobile Web - Chrome
Android.Emulator.-.Pixel_4_API_31_5554.2023-04-05.22-14-56.mp4
Mobile Web - Safari
Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-04-06.at.19.20.07.mp4
Desktop
Screen.Recording.2023-04-06.at.19.17.18.mov
iOS
Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-04-06.at.19.12.30.mp4
Android
Android.Emulator.-.Pixel_4_API_31_5554.2023-04-05.21-58-57.mp4