Attachment Carousel Virtual Caching #16973
+144
−137
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,79 +1,4 @@ | ||
| // No need to implement this in native, because all the native actions (swiping) are handled by the parent component | ||
| const Carousel = () => {}; | ||
|
|
||
| export default Carousel; |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| import React from 'react'; | ||
| import {View, FlatList, Pressable} from 'react-native'; | ||
| import PropTypes from 'prop-types'; | ||
| import {withOnyx} from 'react-native-onyx'; | ||
| import _ from 'underscore'; | ||
|
|
@@ -43,14 +43,25 @@ class AttachmentCarousel extends React.Component { | |
| constructor(props) { | ||
| super(props); | ||
|
|
||
| this.scrollRef = React.createRef(); | ||
| this.canUseTouchScreen = DeviceCapabilities.canUseTouchScreen(); | ||
| this.viewabilityConfig = { | ||
| // To facilitate paging through the attachments, we want to consider an item "viewable" when it is | ||
| // more than 90% visible. When that happens we update the page index in the state. | ||
| itemVisiblePercentThreshold: 95, | ||
| }; | ||
|
|
||
| this.cycleThroughAttachments = this.cycleThroughAttachments.bind(this); | ||
| this.getItemLayout = this.getItemLayout.bind(this); | ||
| this.renderItem = this.renderItem.bind(this); | ||
| this.renderCell = this.renderCell.bind(this); | ||
| this.updatePage = this.updatePage.bind(this); | ||
|
|
||
| this.state = { | ||
| attachments: [], | ||
| source: this.props.source, | ||
| shouldShowArrow: this.canUseTouchScreen, | ||
| containerWidth: 0, | ||
| }; | ||
| } | ||
|
|
||
|
|
@@ -64,6 +75,7 @@ class AttachmentCarousel extends React.Component { | |
| if (previousReportActionsCount === currentReportActionsCount) { | ||
| return; | ||
| } | ||
|
|
||
| this.makeStateWithReports(); | ||
| } | ||
|
|
||
|
|
@@ -75,14 +87,27 @@ class AttachmentCarousel extends React.Component { | |
| getAttachment(attachmentItem) { | ||
| const source = _.get(attachmentItem, 'source', ''); | ||
| const file = _.get(attachmentItem, 'file', {name: ''}); | ||
|
|
||
| return { | ||
| source, | ||
| file, | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Calculate items layout information to optimize scrolling performance | ||
| * @param {*} data | ||
| * @param {Number} index | ||
| * @returns {{offset: Number, length: Number, index: Number}} | ||
| */ | ||
| getItemLayout(data, index) { | ||
| return ({ | ||
| length: this.state.containerWidth, | ||
| offset: this.state.containerWidth * index, | ||
| index, | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Toggles the visibility of the arrows | ||
| * @param {Boolean} shouldShowArrow | ||
|
|
@@ -92,10 +117,10 @@ class AttachmentCarousel extends React.Component { | |
| } | ||
|
|
||
| /** | ||
| * Map report actions to attachment items | ||
| */ | ||
| makeStateWithReports() { | ||
| let page = 0; | ||
| const actions = ReportActionsUtils.getSortedReportActions(_.values(this.props.reportActions), true); | ||
|
|
||
| /** | ||
|
|
@@ -124,50 +149,96 @@ class AttachmentCarousel extends React.Component { | |
| } | ||
| }); | ||
|
|
||
| this.setState({ | ||
| page, | ||
| attachments, | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Increments or decrements the index to get another selected item | ||
| * @param {Number} deltaSlide | ||
| */ | ||
| cycleThroughAttachments(deltaSlide) { | ||
| const nextIndex = this.state.page - deltaSlide; | ||
| const nextItem = this.state.attachments[nextIndex]; | ||
|
|
||
| if (!nextItem) { | ||
| return; | ||
|
Comment on lines
-142
to
167
There was a problem hiding this comment. the (This also lets us remove |
||
| } | ||
|
|
||
| // 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}); | ||
|
Comment on lines
-146
to
+172
There was a problem hiding this comment. The state update was moved to Here we only tell the view to scroll, which in turn would trigger |
||
| } | ||
|
|
||
| /** | ||
| * Updates the page state when the user navigates between attachments | ||
| * @param {Array<{item: *, index: Number}>} viewableItems | ||
| */ | ||
| updatePage({viewableItems}) { | ||
| // Since we can have only one item in view at a time, we can use the first item in the array | ||
| // to get the index of the current page | ||
| const entry = _.first(viewableItems); | ||
| if (!entry) { | ||
| return; | ||
| } | ||
|
|
||
| const page = entry.index; | ||
| const {source, file} = this.getAttachment(entry.item); | ||
| this.props.onNavigate({source: addEncryptedAuthTokenToURL(source), file}); | ||
| this.setState({page, source}); | ||
| } | ||
|
|
||
| /** | ||
| * Defines how a container for a single attachment should be rendered | ||
| * @param {Object} props | ||
| * @returns {JSX.Element} | ||
| */ | ||
| renderCell(props) { | ||
| const style = [props.style, styles.h100, {width: this.state.containerWidth}]; | ||
|
|
||
|
|
||
| // 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} | ||
| /> | ||
| ); | ||
|
Comment on lines
+201
to
+215
There was a problem hiding this comment. This is something I picked up while reviewing the Having the |
||
| } | ||
|
|
||
| /** | ||
| * Defines how a single attachment should be rendered | ||
| * @param {{ source: String, file: { name: String } }} item | ||
| * @returns {JSX.Element} | ||
| */ | ||
| renderItem({item}) { | ||
| const authSource = addEncryptedAuthTokenToURL(item.source); | ||
| return <AttachmentView source={authSource} file={item.file} />; | ||
| } | ||
|
|
||
| render() { | ||
| const isForwardDisabled = this.state.page === 0; | ||
| const isBackDisabled = this.state.page === _.size(this.state.attachments) - 1; | ||
|
|
||
| return ( | ||
| <View | ||
| style={[styles.attachmentModalArrowsContainer, styles.flex1]} | ||
| onLayout={({nativeEvent}) => this.setState({containerWidth: nativeEvent.layout.width + 1})} | ||
|
There was a problem hiding this comment. 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 |
||
| onMouseEnter={() => this.toggleArrowsVisibility(true)} | ||
| onMouseLeave={() => this.toggleArrowsVisibility(false)} | ||
| > | ||
| {this.state.shouldShowArrow && ( | ||
| <> | ||
| {!isBackDisabled && ( | ||
| <View style={styles.leftAttachmentArrow}> | ||
| <Tooltip text={this.props.translate('common.previous')}> | ||
| <Button | ||
|
|
@@ -181,7 +252,7 @@ class AttachmentCarousel extends React.Component { | |
| </Tooltip> | ||
| </View> | ||
| )} | ||
| {!isForwardDisabled && ( | ||
| <View style={styles.rightAttachmentArrow}> | ||
| <Tooltip text={this.props.translate('common.next')}> | ||
| <Button | ||
|
|
@@ -197,20 +268,47 @@ class AttachmentCarousel extends React.Component { | |
| )} | ||
| </> | ||
| )} | ||
|
There was a problem hiding this comment. Follow up refactor - the There are no more usages of AttachmentView with |
||
|
|
||
| {this.state.containerWidth > 0 && ( | ||
| <FlatList | ||
|
There was a problem hiding this comment. Coming from #19484: |
||
| listKey="AttachmentCarousel" | ||
| horizontal | ||
|
|
||
| // Inverting the list for touchscreen devices that can swipe or have an animation when scrolling | ||
| // promotes the natural feeling of swiping left/right to go to the next/previous image | ||
| // We don't want to invert the list for desktop/web because this interferes with mouse | ||
| // wheel or trackpad scrolling (in cases like document preview where you can scroll vertically) | ||
| inverted={this.canUseTouchScreen} | ||
|
|
||
| decelerationRate="fast" | ||
| showsHorizontalScrollIndicator={false} | ||
| bounces={false} | ||
|
|
||
| // Scroll only one image at a time no matter how fast the user swipes | ||
| disableIntervalMomentum | ||
| pagingEnabled | ||
| snapToAlignment="start" | ||
| snapToInterval={this.state.containerWidth} | ||
|
|
||
| // Enable scrolling by swiping on mobile (touch) devices only | ||
| // disable scroll for desktop/browsers because they add their scrollbars | ||
| scrollEnabled={this.canUseTouchScreen} | ||
| ref={this.scrollRef} | ||
| initialScrollIndex={this.state.page} | ||
| initialNumToRender={3} | ||
| windowSize={5} | ||
| maxToRenderPerBatch={3} | ||
| data={this.state.attachments} | ||
| CellRendererComponent={this.renderCell} | ||
| renderItem={this.renderItem} | ||
| getItemLayout={this.getItemLayout} | ||
| keyExtractor={item => item.source} | ||
| viewabilityConfig={this.viewabilityConfig} | ||
| onViewableItemsChanged={this.updatePage} | ||
| /> | ||
| )} | ||
|
|
||
| <CarouselActions onCycleThroughAttachments={this.cycleThroughAttachments} /> | ||
| </View> | ||
| ); | ||
| } | ||
|
|
||
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.
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
updatePagemethod - it shouldn't be in a getter