Skip to content
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.

Sign up for GitHub

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

#23546 - Wrong Image opens on HT Report in Safari #28441

Closed
wants to merge 8 commits into from
Closed

#23546 - Wrong Image opens on HT Report in Safari #28441

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
35ae16a
#23546 - Wrong Image opens on HT Report in Safari
mikesmnx Sep 27, 2023
3d4faa6
pagingEnabled removed
mikesmnx Sep 29, 2023
d1bf919
Updated approach
mikesmnx Oct 12, 2023
afcac5d
Merge remote-tracking branch 'upstream/main' into mikesmnx-issue-23546
mikesmnx Oct 23, 2023
d10f9fd
Fixed lint error
mikesmnx Nov 2, 2023
6f41ef5
Prettier fix
mikesmnx Nov 2, 2023
60ac519
Merge remote-tracking branch 'upstream/main' into mikesmnx-issue-23546
mikesmnx Dec 8, 2023
5a19e1f
Added comment on contentInset
mikesmnx Dec 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions src/components/Attachments/AttachmentCarousel/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ const viewabilityConfig = {
// To facilitate paging through the attachments, we want to consider an item "viewable" when it is
// more than 95% visible. When that happens we update the page index in the state.
itemVisiblePercentThreshold: 95,
minimumViewTime: 300,
};

function AttachmentCarousel({report, reportActions, parentReportActions, source, onNavigate, setDownloadButtonVisibility, translate, transaction}) {
Expand Down Expand Up @@ -195,16 +196,19 @@ function AttachmentCarousel({report, reportActions, parentReportActions, source,
disableIntervalMomentum
pagingEnabled
snapToAlignment="start"
snapToInterval={containerWidth}
// Without explicitly setting this value (despite it being the default),
// the view may appear slightly different (with a 1-2px padding)
contentInset={{top: 0, left: 0, bottom: 0, right: 0}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be default value. Any reason for adding this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I recall from my tests on platforms, such as macOS desktop and iOS native, I found that without explicitly setting this value (despite it being the default), the view appeared slightly different (with a 1-2px padding)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add that as a comment.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

contentInsetAdjustmentBehavior="automatic"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this? Can you please explain the issue it solves?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check my comment here

snapToOffsets={_.map(attachments, (__, index) => index * containerWidth)}
// Enable scrolling by swiping on mobile (touch) devices only
// disable scroll for desktop/browsers because they add their scrollbars
// Enable scrolling FlatList only when PDF is not in a zoomed state
scrollEnabled={canUseTouchScreen}
ref={scrollRef}
initialScrollIndex={page}
initialNumToRender={3}
windowSize={5}
maxToRenderPerBatch={CONST.MAX_TO_RENDER_PER_BATCH.CAROUSEL}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did we remove this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recently, the 'maxToRenderPerBatch' was set to use a constant, but we have removed it in this PR

initialNumToRender={1}
windowSize={1}
data={attachments}
CellRendererComponent={AttachmentCarouselCellRenderer}
renderItem={renderItem}
Expand Down
Loading