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.

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

Carousel - Use measured width/height if it exists. #915

Merged

Conversation

ekimmel-ta
Copy link
Contributor

My carousel component is not full width. The current carousel implementation was defaulting to the full screen width because layout had not completed yet.

This PR adds measured width / measured height to the getTotalWidthPx and getTotalHeightPx functions in Carousel to help prevent a situation where the carousel is not using the intending width.

@elihart
Copy link
Contributor

elihart commented May 5, 2020

Sounds good to me :)

@elihart elihart merged commit aa3749a into airbnb:master May 5, 2020
@elihart elihart mentioned this pull request May 15, 2020
@andybdahl
Copy link

This merge created the issue mentioned in the issue #682 . A proper layout should be done by only using the view's width / measured height as long as the orientation stays the same, and should default to the screen width OR measure the Carousel once again after configuration changes has been made.

@elihart
Copy link
Contributor

elihart commented Feb 11, 2021

@andybdahl thanks for catching this. are you interested in contributing that fix?

@andybdahl
Copy link

andybdahl commented Feb 12, 2021 via email

@ekimmel-ta
Copy link
Contributor Author

Hi, could you explain how this PR affects your issue? Epoxy was already using this.width previously, and as you said that value is wrong during configuration changes.

I did find that Epoxy reusing a Carousel model with a different width, even without configuration changes, is still broken. Width is the old value and measuredWidth is the correct, updated value. I don't know how we could detect that width is out of date.

@ekimmel-ta
Copy link
Contributor Author

Slight update: Using width before measuredWidth is incorrect if this function is called during onMeasure (meaning during onChildAttachedToWindow as well). I don't know if that means we should be using measuredWidth only or if we need to flag when we're in the measurement phase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants