sag1v / react-elastic-carousel

A flexible and responsive carousel component for react https://sag1v.github.io/react-elastic-carousel
MIT License
348 stars 150 forks source link

Height Bug #54

Closed waynehaffenden closed 4 years ago

waynehaffenden commented 4 years ago

This is a bit of a strange one and I might be going about it the wrong way here so advice is appreciated. I've got different height items within the carousel, now what I'm trying to do is make them all the same height but this seems to be really difficult! Here's the CSS I have so far:

  & .rec-slider {
    display: flex;
    /*height: 100%;*/
    min-height: auto;
  }

  & .rec-slider > [tabindex] {
    height: 100%;
    outline: none;
  }

  & .rec-slider [class*="rec-swipable-"],
  & .rec-item-wrapper {
    height: 100%;
  }

Now if I use Chrome dev tools and set the height to 100% in .rec-slider div then it works great and is exactly what I'm looking for. However, if I uncomment the line in the CSS above in .rec-slider (same thing as in Chrome dev tools) then nothing is visible and the actual height is 0px!? This appears to be something that happens within the Carousel component, any ideas?

Please help, thank you!

sag1v commented 4 years ago

@waynehaffenden Can you provide a running example?

waynehaffenden commented 4 years ago

@sag1v Please check the demo here: https://codesandbox.io/s/suspicious-fire-wfc4b?file=/src/App.js. You'll see when you load the items within the carousel are different heights, if you then uncomment the height: 100% within the .rec-slider class you'll see it works as expected. Then refresh the page or browser window (with the height uncommented) and you'll see that none of the items within the carousel are now visible.

Any ideas?

sag1v commented 4 years ago

@waynehaffenden The carousel element is initialised with a 0 height, then it subscribing to the resize event of the slider item and sets its height accordingly.

I think that when you explicit set the slider element's height to 100% (ie. take the height of the parent) then you break that flow of the carousel that is "listening" to the height of its child (slider), i mean they both are depending on each other's height in this case.

Why exactly do you need the slider to be height: 100% by the way?

waynehaffenden commented 4 years ago

@sag1v Thanks for the detailed explanation.

In all honesty I don't need the slider to be height: 100%, all I'm trying to do is make all the items within the slider the same height without specifying a height as the contents are dynamic. The only way I was able to make this happen is by setting all the divs within the carousel to height: 100% but doing this on page reload makes the height 0px.

Any suggestions on making all the items within the carousel have the same height without breaking it as it does currently? Thank you for your time.

sag1v commented 4 years ago

@waynehaffenden But what height do you want them to be, by the highest item?

waynehaffenden commented 4 years ago

@sag1v Exactly that

sag1v commented 4 years ago

So i think you should just set the image to 100% width (yeah, width). These rule should be sufficient:

const Wrapper = styled.div`
  background: #efefef;
  padding: 4rem;

  & .rec-slider > [tabindex] {
    outline: none;
  }
`;

const Card = styled.div`
  width: 100%;
  padding: 2rem;
  margin: 0 2rem;
  background: #fff;
`;

const Img = styled.img`
  width: 100%;
`;
waynehaffenden commented 4 years ago

@sag1v That doesn't seem to work and is what I had previously: https://codesandbox.io/s/suspicious-fire-wfc4b?file=/src/App.js

sag1v commented 4 years ago

@waynehaffenden In the example you shared you set the max-width and not the width of the img:

const Img = styled.img`
  max-width: 100%;
`;
waynehaffenden commented 4 years ago

@sag1v Amazing, however, (sorry) see my updated example (this is how it really is): https://codesandbox.io/s/suspicious-fire-wfc4b?file=/src/App.js

sag1v commented 4 years ago

@waynehaffenden OK so the image has a sibling (text or other content), I'm not a CSS expert but i think making all nested elements display:flex will make them fill the height of their parent.

In this case we only need to add this to 2 elements, so i think this should work for you:

const Wrapper = styled.div`
  background: #efefef;
  padding: 4rem;

  & .rec-slider [tabindex], .rec-slider [class*="rec-swipable-"] {
    display: flex;
  }
`;
waynehaffenden commented 4 years ago

@sag1v Thank you, this sadly doesn't work but I'll continue to experiment. I'm closing this issue for now as it seems that it's not a bug. Thanks again

sag1v commented 4 years ago

@waynehaffenden Strange, it works for me. With this code:

const Wrapper = styled.div`
  background: #efefef;
  padding: 4rem;

  & .rec-slider [tabindex], .rec-slider [class*="rec-swipable-"] {
    display: flex;
  }
`;

const Card = styled.div`
  position: relative;
  display: flex;
  flex-direction: column;
  width: 100%;
  padding: 2rem;
  margin: 0 2rem;
  background: #fff;
`;

const Img = styled.img`
  width: 100%;
`;

This is the result i get:

image

Anyway, you might need to play around with the cards styling to get exactly what you after, and as you said, i guess this is not a bug :slightly_smiling_face: