Carousel - fix initial page offset (contentOffset)

[M-i-k-e-l]

Description

Carousel - fix initial page offset (contentOffset)
WOAUILIB-2301
Generally I'd like to replace all calcOffset usages to the new getContentOffset (since snapToOffsets seem to have the correct data) but I fear of edge cases and bugs.
TBH, I think we should rewrite this component as a function component.

Testing snippet:

<Carousel
  onChangePage={this.onChangePage}
  pageWidth={249 + (Spacings.s5 * 2)}
  itemSpacings={Spacings.s5}
  initialPage={1}
  containerStyle={{height: 160}}
>
  {_.map([...Array(numberOfPagesShown)], (item, index) => (
    <Page style={{backgroundColor: BACKGROUND_COLORS[index]}} key={index}>
      <Text margin-15>CARD {index}</Text>
    </Page>
  ))}
</Carousel>
...
page: {
  flex: 1,
  width: 249,
  borderWidth: 1,
  borderRadius: 8
}

Changelog

Carousel - fix initial page offset (contentOffset)

[ethanshar]

Why did you remove onContentSizeChange?

[M-i-k-e-l]

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?

[ethanshar]

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

[ethanshar]

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

[M-i-k-e-l]

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.

[ethanshar]

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?

[M-i-k-e-l]

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?

[ethanshar]

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

[M-i-k-e-l]

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