FormidableLabs / nuka-carousel

Small, fast, and accessibility-first React carousel library with an easily customizable UI and behavior to fit your brand and site.
https://commerce.nearform.com/open-source/nuka-carousel
Other
3.07k stars 595 forks source link

Initial Height too long (broken) #457

Closed rockmandash closed 5 years ago

rockmandash commented 5 years ago

Initial Height too long (broken) initialSlideHeight & heightMode both not helping. Have to wait until next slide, the height will back to normal.

hfmw commented 5 years ago

Duplicate of: https://github.com/FormidableLabs/nuka-carousel/issues/202 @sarmeyer / @rockmandash please close.

zzolain commented 5 years ago

I guess this issue has not been solved yet. when using prop heightMode="max", initialHeight is too long. as @rockmandash mentioned, To get normal height, sliding to another slide is needed.

rayyapari commented 5 years ago

I have same issue, need help :(

haugan commented 5 years ago

Duplicate of: #202 @sarmeyer / @rockmandash please close.

The duplicate is closed, so closing this new one won't help anyone. The problem needs to be addressed.

ghost commented 5 years ago

Reproducible bug https://codesandbox.io/s/q83z6q0636

sarmeyer commented 5 years ago

fixed with #469

eduardoabpereira commented 5 years ago

Please, reopen this issue since initial height isn't fixed well.

sarmeyer commented 5 years ago

@vyteniskuciauskas-home24 I'm not able to reproduce this issue. Is anyone able to send me an example with this issue present?

ghost commented 5 years ago

works for me also Edit q3r3mq980q

eduardoabpereira commented 5 years ago

@sarmeyer and @vyteniskuciauskas-home24 For me, the first render is with long height and after resize it normalize with correct height value. And the first height is something like document height + 100vh

sarmeyer commented 5 years ago

@vyteniskuciauskas-home24 this is what I see when the codesandbox loads: image

The height looks correct, and it doesn't change as I move through the slides. Is there another example you have?

eduardoabpereira commented 5 years ago

@sarmeyer look these screenshots:

before resize, slider apply 5982px of height https://i.imgur.com/sxV9DXx.png

after resize, everything work fine: https://i.imgur.com/KMNRdZw.png

on @vyteniskuciauskas-home24 sandbox the problem appears for me too.

before resize do not render slider content: https://i.imgur.com/JXErKqt.png

after resize everything work fine: https://i.imgur.com/b03lTrz.png

sarmeyer commented 5 years ago

@devedupereira what version of nuka are you using? Thanks for helping me pin this down, I'm still not able to reproduce it locally, so your screenshots help!

eduardoabpereira commented 5 years ago

@sarmeyer latest version 4.4.5

sarmeyer commented 5 years ago

Does anyone have a repo I can pull down with this issue? I am still not able to reproduce on my system outside of codesandbox. I must be missing something here

Caldwerl commented 5 years ago

I will add that this is occurring for myself in V 4.4.5, not on first page load, but after I navigate into a view with a carousel. First page load is fine, perfect, if I navigate to a page with a carousel, it has over double the height until the next slide.

So most likely you wont be recreating this on a single view version.

My guess is it could be related to the setDimensions call in componentDidMount after it is first initialized.

clayhan commented 5 years ago

I am also experiencing this and am hacking my way through it by targeting the ul inside of the carousel by giving it a max-height.

For me, I am noticing this issue when I am routing client side from one page to another. When I route to a page client side, the ul that holds all of the list items has a massive height. Once I move the browser window even the slightest bit, everything snaps to normal.

capture

povilaspin commented 5 years ago

Having totally the same issue like @clayhan

flikQ commented 5 years ago

Is this still being looked at by any chance @sarmeyer ?

StevenElberger commented 5 years ago

I know it's not a fix, but if all slides are the same height or the current slide is largest, changing heightMode to current should do the trick for you.

SummitPatel commented 5 years ago

I'm still having this issue with v4.4.7.

Temporary fix was to wrap my Review component (i.e. the slides of the carousel) in a div with a height set:

export const ReviewsCarousel = props => (
  <Carousel heightMode={"max"}>
    {props.reviews.length &&
      props.reviews.map((review, i) => {
        return (
          <div style={{ height: 200 }}>
            <Review
              key={i}
              content={review.attributes.review}
              rating={review.attributes.rating}
            />
          </div>
        );
      })}
  </Carousel>
);
povilaspin commented 5 years ago

@sarmeyer is this is looked at? Pretty critical and one-and-only bug that is stopping me from starting to go nuts with it. I've created a sandbox for you where you should be able to recreate that. Happens on initial load, after interacting with it it goes back to normal https://codesandbox.io/s/v0n97j1yq3 . I can't put initialHeight due to the dynamic content that the slide might have. Can't put width either - i'd need '25%' or something similar, but only 'auto' and integers are viable

sarmeyer commented 5 years ago

@povilaspin Hi, I'm working on this now. I can see the issue in the codesandbox you sent me...I realize this is high priority and I'll try to get a fix out asap

clayhan commented 5 years ago

Thanks for all your work on this @sarmeyer, do you possibly have a pulse on if this fix is tough/easy? I have to get a release going in about a week and would like to pivot off the package if you felt this was going to be a beast. Thanks a ton!

flikQ commented 5 years ago

Any updates on this issue?

x77t commented 5 years ago

Hey @sarmeyer, so I kind of found the bugs that are causing these problems with height. There are 2 main problem I found: First .slider-list and .slider-slide had bad width and because of that the content inside was squeezed and so the .slider-slide height was stretched, to fix this all I did was set width of .slider-list and .slider-slide to 100%!important; Second problem is that slides with images have this problem that the image loading is not in sync with your height calculation and thats also the problem why some people have slider height: 0px, for this problem I did this window.onload = () => carousel.props.onResize(); (bad example) to wait for images to load and to recalculate the sizes.

jamesmosier commented 5 years ago

I found that initialSlideHeight had no effect and when I would dynamically re-render a carousel and that new carousel would have a height of 18px. To fix this, I set heightMode={null} and then set my initialSlideHeight value.

I believe the reason I had to do this was because heightMode is defaulted to max therefore when the height is calculated it can never make it to this line because this conditional is truthy.


Another work around I implemented which worked maybe even better was to set padding-bottom on .slider-list which ensures the slider has the proper height for the aspect ratio. This way I didn't need to set an initialSliderHeight (or width). For example:

<NukaCarousel heightMode="current">...</NukaCarousel>
.slider-list {
  padding-bottom: 77.31959% !important;
}
S33G commented 5 years ago

I'm giving up using this carousel because of this problem. This should be pointed out in docs to avoid other devs losing time on this :(

Blobson commented 5 years ago

Slightly modified version of @jamesmosier`s fix with fixed height and 16/9 aspect ratio:

import NukaCarousel from 'nuka-carousel'
import styled from 'styled-components'

const MyCarousel = styled(NukaCarousel)`
  & .slider-list {
    height: 0 !important;
    padding-bottom: 56.25% !important;
  }
`

export default MyCarousel

The main difference is that carousel height will be preserved after slide change. Note that you don't need to change NukaCarousel`s heightMode parameter.

Thanks to @jamesmosier for the initial tip, I think it's a candidate for a separate manual section.

davemeyer commented 5 years ago

terrible workaround, but i've been using an 'empty' first slide and then skipping past it:

export default () => {
const [slideIndex, setSlideIndex] = useState(0);
  useEffect(() => {
    if (slideIndex === 0) {
      setSlideIndex(1);
    }
  }, [slideIndex]);
...
return (
  <Carousel slideIndex={slideIndex} />
    <div />
    <ActualComponent1 />
    <ActualComponent2 />
  </ Carousel>

probably isn't a good option for all usecases, but works fine for my single-direction non-wrapping scenario.

flikQ commented 5 years ago

@charj I have also decided to ditch this library in favour of React-Slick which works well.

sarmeyer commented 5 years ago

With the latest release of 4.5.6, I believe this issue is fixed. Can I get some help confirming that in your own environments?

Thanks!

michaelcfl commented 5 years ago

Thanks for the update!

It is kinda weird for me because this issue is not reproducible all the time. It happens sometimes. I personally think that this may related to the child inside the carousel. The carousel won't calculate the height upon addition of child. The code below is the common component for carousel in my website.

(!!photos && <Carousel
       className={imageContainerClass}
       beforeSlide={slideIndex => setIndex(slideIndex)}
       renderCenterLeftControls={({ previousSlide }) => (
         <IconButton onClick={previousSlide} color="secondary">
           <MxIcon icon="left" />
         </IconButton>
       )}
       renderCenterRightControls={({ nextSlide }) => (
         <IconButton onClick={nextSlide} color="secondary">
           <MxIcon icon="right" />
         </IconButton>
       )}
       renderBottomCenterControls={null}
       wrapAround={!!wrapAround ? wrapAround : false}
       slidesToShow={!!slidesToShow ? slidesToShow : 1}
       slideIndex={index}
       dragging={false}
     >
       {
         photos.map(photo => (
           <img key={photo} className={imageClass} src={photo} />
       ))}
</Carousel>)

However, my case could be my code problem because the reproducible codeine is fixed after updating the library.

jalada commented 5 years ago

I can confirm that this issue is fixed for me when testing with 4.5.8. I did have to set an initialSlideWidth to make it render a little nicer at first before the calculations take effect on page load. That might be more a next.js SSR issue though.

sarmeyer commented 5 years ago

continuing any lingering investigation in this ticket https://github.com/FormidableLabs/nuka-carousel/issues/521