Skip to content

Carousel - fix initial page offset (contentOffset) #1991

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

Merged
merged 4 commits into from
Apr 28, 2022
Merged

Carousel - fix initial page offset (contentOffset) #1991

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
56a4d5a
Carousel - fix initial page offset (contentOffset)
M-i-k-e-l Apr 18, 2022
311cc11
Merge branch 'master' into fix/carousel-initial-page-offset
M-i-k-e-l Apr 25, 2022
ddd435e
Add old code as comment
M-i-k-e-l Apr 25, 2022
cc5c435
Rename to getInitialContentOffset
M-i-k-e-l Apr 28, 2022
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
1 change: 1 addition & 0 deletions src/components/carousel/CarouselPresenter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ export function getChildrenLength(props: PropsWithChildren<CarouselProps>): numb
return React.Children.count(props.children);
}

// TODO: This should probably be replaced with carousel.getContentOffset
export function calcOffset(props: CarouselProps, state: Omit<CarouselState, 'initialOffset' | 'prevProps' | 'currentStandingPage'>) {
const {currentPage, pageWidth, pageHeight} = state;
const {loop, containerMarginHorizontal = 0} = props;
Expand Down
21 changes: 18 additions & 3 deletions src/components/carousel/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,21 @@ class Carousel extends Component<CarouselProps, CarouselState> {
}
};

getInitialContentOffset = (snapToOffsets: number[] | undefined) => {
const {horizontal, initialPage} = this.props;
const {initialOffset} = this.state;
let contentOffset = initialOffset;
if (snapToOffsets && initialPage !== undefined) {
const offset = snapToOffsets[initialPage];
contentOffset = {
x: horizontal ? offset : 0,
y: horizontal ? 0 : offset
};
}
Comment on lines +253 to +259
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't this code be part of what we store in the initialOffset? in the onContainerLayout method?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do know this is a bit hacky, as I've addressed in my description, there are other places where we should probably change it (such as the constructor), but TBH if we want to change them then I feel like this conversation is going towards: let's rewrite this component.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure it has to go that way.
We have a method that calcOffset, we use it for setting the initial offset.
Now you the logic for initial offset changed a little, Can't we create a method for calcInitialOffset that returns the relevant offset and set it in the state?
What are the blockers here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The new method (with the new logic) getContentOffset depends on snapToOffsets which is calculated using containerWidth which is obtained in onContainerLayout.
Technically from testing now, it seems that only the constructor is occurring before this happens, but it feels unsafe to me, WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, I see your point.
So can we rename the new method to getInitialContentOffset so it's clear that it's the initial offset

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, but I blame RN:

/**
 * Used to manually set the starting scroll offset.
 * The default value is {x: 0, y: 0}
 */
contentOffset?: PointPropType | undefined; // zeros


return contentOffset;
}

shouldUsePageWidth() {
const {loop, pageWidth} = this.props;
return !loop && pageWidth;
Expand Down Expand Up @@ -477,13 +492,13 @@ class Carousel extends Component<CarouselProps, CarouselState> {

renderCarousel() {
const {containerStyle, animated, horizontal, animatedScrollOffset, ...others} = this.props;
const {initialOffset} = this.state;
const scrollContainerStyle = this.shouldUsePageWidth()
? {paddingRight: this.getItemSpacings(this.props)}
: undefined;
const snapToOffsets = this.getSnapToOffsets();
const marginBottom = Math.max(0, this.getContainerPaddingVertical() - 16);
const ScrollContainer = animatedScrollOffset ? Animated.ScrollView : ScrollView;
const contentOffset = this.getInitialContentOffset(snapToOffsets);
return (
<View
animated={animated}
Expand All @@ -502,8 +517,8 @@ class Carousel extends Component<CarouselProps, CarouselState> {
horizontal={horizontal}
pagingEnabled={this.shouldEnablePagination()}
snapToOffsets={snapToOffsets}
contentOffset={initialOffset} // iOS only
onContentSizeChange={this.onContentSizeChange}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you remove onContentSizeChange?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that contentOffset={initialOffset} // iOS only was actually only working for iOS and it looks like it's working on Android as well now; and I think that onContentSizeChange was there to workaround that issue.
Do you know of a case where this is useful?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, but let's keep it commented for now, so if an issue will rise soon it'll be easier to spot it.
If in the future we'll come across it we'll remove it

contentOffset={contentOffset}
// onContentSizeChange={this.onContentSizeChange}
onMomentumScrollEnd={this.onMomentumScrollEnd}
>
{this.renderChildren()}
Expand Down