[Components]: Add AspectRatio #33603
Conversation
ntsekouras
added
[Package] Components
|
Size Change: +233 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
I'm a bit confused by the choice of having width as a prop:
- it is actually used as the
maxWidthinternally, which may cause confusion - if we're exposing the
widthprop, why notheight?
Do we have any insights on why this choice was made in the first place?
I'd be almost tempted to remove the width property and have the consumer of this component set a width / height via CSS.
Co-authored-by: Marco Ciampini <[email protected]>
I don't have any 😄 .
Sounds good, I agree with this. Do you want to get some other opinion on that? If not I can make the changes. |
Probably a good idea to hear @sarayourfriend snd @diegohaz 's opinions too |
Wouldn't you set the width/height on the |
I had a deeper look at how this component works. From what I understood:
I believe that this way of enforcing an aspect ratio only works with setting the While looking for other examples of similar components, I also found https://www.npmjs.com/package/react-aspect-ratio — where @sgomes is mentioned as the owner of the "original idea". This component, in a similar way to what we were discussing, only exposes the |
|
Thank you for the mention, @ciampo ! I just want to point out that all these aspect ratio tricks, while extremely cool, are pretty old and were designed to work around fundamental browser limitations that no longer apply today, now that we have CSS Since support for the feature seems to cover all the browsers we currently officially support, perhaps we should use that instead? |
I want to clarify I am in no way the author of the original idea of the padding trick, which as far as I know came from Thierry Koblentz. All I did was package it up into a silly CSS module to illustrate my idea of "CSS custom properties as your API" 🙂 |
That sounds like a great idea, given that we don't officially support IE anymore!
Thank you for the clarification! I just quoted what was written in the npm package's README 😅 |
Haha, no worries, I just wanted to make sure I don't get undue credit from anyone following the thread 😄 |
|
Thanks for the suggestion @sgomes and for the reviews @sarayourfriend and @ciampo ! I've updated the code with the new approach. |
There was a problem hiding this comment.
Now that we're using the aspect-ratio CSS rule, I wonder if having the AspectRatioView as a wrapper around the child is necessary at all.
It also has the side-effect of introducing a small gap under its content
Also, have we agreed on removing the width prop?
If we don't, isn't it just a css prop then? Is this component will be needed? |
That's a legit question — probably the reason why other similar components like I'm personally not sure if there is value in keeping What do y'all think about this? |
I guess this could have value if we keep the wrapper, render all children and attach the |
|
After discussing this with @sarayourfriend, we feel that there isn't a real need for this to be a separate component, now that its functionality can be replaced by the We should proceed with #33488 and try to implement the Sorry, @ntsekouras, for the time that you spent working on this PR! At the same time, I feel like this has been nonetheless a very useful conversation! |
No worries at all! The outcome from this PR (decide to use css) was way better than introducing an older way of achieving the same result 😄 |
Description
Part of: #30503
Needed for: #33488
Adds a new AspectRatio component.