-
Notifications
You must be signed in to change notification settings - Fork 161
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
Full height carousel #94
Comments
I've removed extra wrapper |
I am closing this and keeping your other one open. |
Honestly, I'd prefer the other way around. |
As I refactor my code, I reconsider how I use the component. I need the carousel to be actually responsive, and by that I mean it should take 100% of both width and height of a container. If I'd need some constrains like width/height ratio, I would easily apply it to a wrapper div. But with the current API, I have to fight with required width/height ratio by introducing the workaround posted above. Why do I need to complicate my code with a hairy stateful component? I propose to make these height/width props optional. Will it be two values, as it is now (width and height) or a single ratio, it doesn't matter, but I think a ratio is way less confusing. Just try to make a 100vh/100vw carousel with this component to get a feeling of "fighting some other developer's CSS or DOM structure", something the readme promises will be avoided. :) |
I understand what you are saying. I am not sure I have a good answer for you. The problem with 100vh/100vw is that it isn't a ratio (which is what you are proposing). However, I also agree that making a full screen carousel shouldn't be difficult. Is that ultimately what you are trying to achieve? Do you mind sharing a code sample of what you have and what you are doing in order to achieve this? |
Quoting myself,
I'm glad you share my concern that 100vh/100vw should be easy. Here's my code. You can actually run the project ( |
Half-baked refactored version is here. |
@mvasin I just want to clarify something: I said I agree that making a full screen carousel shouldn't be difficult. 100vh/100vw is not a ratio because vh/vw aren't sepcific units of measurement, they are relative units of measurement, and 100vh/vw may not be the answer. I wanted to clarify the goal: making a carousel that takes up the full width and full height of the screen. |
100vh/100vw I used as an example of unpredictable ratio, in other words, no ratio. And controlling the carousel wrapper size/dimensions seems to be the most straightforward way to control the carousel dimensions; the carousel will perfectly fit any responsive environment, and making a wrapper of a fixed ratio is still an option. I propose to make the carousel take 100% of its container node (not necessarily the screen), both vertically and horizontally. That could be the sizing API. If you feel that you still need ratio prop, it may be, but let it be optional. |
Hi |
So, I personally believe that the wrapper @mvasin created is the way to go. We can turn this into a higher order component. I like creating an HOC instead appending single-use props to CarouselProvider's signature. The HOC could accept other conditions besides just full viewport height and width. It could accept percentages. For example, if you wanted to mimic Youtube's portrait view on mobile devices, you'd set your carousel to be 100% viewport width and 33% viewport height. Plus the HOC could optionally watch a target container by ID just in case someone has needs beyond just viewport. I can whip something up. |
Did this ever eventuate? |
Heyo, I was trying to use this component, but the use of the naturalHeight naturalWidth props is very very confusing... also as stated in this issue, sometimes you just want a carousel that fills the entire screen so far I've been stumbling around the myriad of possible props I can pass to this component (intrinsicHeight?) and nothing seems to work It seems like when created the focus was a very specific use case in mind and has been retro fitted to appeal to the broadest possible cases anyways, I'm about to give up and try a different component, but maybe someone can point out to me if they actually managed to get this working as a full height component? |
I would also like to know if this happened |
Is there a way to make a fullsize carousel yet? |
I had to create a carousel that takes 100% space both vertically and horizontally. Here's the wrapper I came up with:
The use case is so common (I guess) that it would be better to make
naturalSlideWidth
/naturalSlideHeight
optional. If they are omitted, the carousel would occupy all the outer space available.If we will make
naturalSlideWidth
/naturalSlideHeight
optional, there is a catch that should be mentioned in the readme: the containing element must have its dimensions specified.The text was updated successfully, but these errors were encountered: