-
Notifications
You must be signed in to change notification settings - Fork 593
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
Resizing height issue #521 #573
Conversation
@@ -199,8 +197,6 @@ In componentDidMount, the initial dimensions are assigned to each slide: | |||
|
|||
Once the component has completed mounting with the accurate width, it waits for the readyStateChange event to fire before measuring the desired height of the content (`current`, `first`, `max`). That measurement then replaces `initialSlideHeight` with the measured height in pixels. | |||
|
|||
If the readyStateChange event fires before the component completes mounting, the height of the component is not measured until a resize event or a change in slide is triggered. | |||
|
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.
I think there should at least be a helpful disclaimer explaining if setDimensions
is run before, let's say, an image loads, then the slide height is not measured again until a subsequent resize event occurs. I have run into this issue before and found a hard workaround, unless this PR truly fixes that issue.
For example, I currently must to do something like this with React refs in my codebase, otherwise the image is completely cut off:
<img onLoad={this.carouselRef.setDimensions()} />
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.
That was the intent of this PR! I did my best to reproduce the issue, but I am not certain if it was sufficient. Did you find any differently?
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.
Nice! @paulathevalley Let me get back to you on this, sorry for the delay. I'll update to latest version and see if your PR fixes issue. Will provide screenshots.
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.
@paulathevalley @sarmeyer Still an issue D:
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.
noooo!! Do you have more details on how you reproduced it? Were you able to reproduce it locally consistently? I would love to understand more clearly.
Description
This PR addresses this resizing height issue documented from the README:
It does so by tracking the
readystatechange
listener in addition to whether the component is mounted (throughthis.mounted
). If both are true, and there is a mismatch in the height, thencomponentDidUpdate
will run again, and the Carousel will recalculate the height and width.I think this solution gets at the symptoms rather than the underlying issue, which is an ordering problem with two events that.. as far as I’m aware, are outside our control. I would like to spend more time on a more elegant solution, but this may suffice for fixing bugs in the meantime.
Fixes #521
Type of Change
Please delete options that are not relevant.
How Has This Been Tested?
I was having trouble reproducing the issue locally, but I was able to reproduce it consistently by commenting out
this.setDimensions()
inonReadyStateChange()
.Checklist: (Feel free to delete this section upon completion)