ResponsiveWrapper: use aspect-ratio CSS prop and support SVG elements #48573
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@aaronrobertshaw and @tomdevisser , can you have a quick look at this PR and let me know if this approach looks promising to you, as an alternative to #48307 ? If it looks promising, I can refine the PR and mark it ready for review. Thank you! |
|
Size Change: -25 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
aaronrobertshaw
approved these changes
Feb 28, 2023
There was a problem hiding this comment.
Thanks for putting together this alternate approach @ciampo 👍
I like the removal of the resize observer and the overall simplification of the component. Nice work!
For the SVG to scale and be contained within the wrapper it needs the viewbox and preserveAspectRatio attributes. Would there be any benefit in noting that in the component's readme? It's not really the wrapper's responsibility but I'm not sure where else we might be able to flag that.
Given this PR's approach covers the vast majority of use cases and provides a performance bump, I think we should move forward with it.
|
@ciampo This looks great, I really disliked the padding-bottom solution, but am too new to this project to just remove it cause I thought it might break with certain use cases or give backwards compat issues. But I'm all for this approach. |
This is still very much a possibility at this point, we should make sure we give this PR a good round of testing! The |
ciampo
added
[Package] Components
ciampo
force-pushed
the
feat/responsive-wrapper-SVG
branch
from
d388cbe to
5382c98
Compare
|
@ciampo It looks like it is breaking at this moment when using my SVG. I'll attach the SVG here so you can use it for testing as well. It is one exported by Adobe Illustrator, so a format that'll be used a lot.
Here's the SVG code<?xml version="1.0" encoding="utf-8"?>
<!-- Generator: Adobe Illustrator 27.1.1, SVG Export Plug-In . SVG Version: 6.00 Build 0) -->
<svg version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px"
viewBox="0 0 900 900" style="enable-background:new 0 0 900 900;" xml:space="preserve">
<style type="text/css">
.st0{fill:none;stroke:#48B3C3;stroke-width:2;stroke-miterlimit:10;}
.st1{fill:#233463;}
.st2{fill:none;stroke:#47B3C2;stroke-width:2;stroke-miterlimit:10;}
.st3{fill:#233463;stroke:#233463;stroke-miterlimit:10;stroke-dasharray:3,3;}
.st4{fill:none;stroke:#3DDE96;stroke-width:20;stroke-miterlimit:10;}
.st5{fill:none;stroke:#3DDE96;stroke-width:10;stroke-miterlimit:10;}
.st6{fill:none;stroke:#47B3C2;stroke-width:20;stroke-miterlimit:10;}
.st7{fill:#5547C2;stroke:#5547C2;stroke-width:20;stroke-miterlimit:10;}
.st8{fill:#5547C2;}
.st9{fill:#48B3C3;}
.st10{fill:#EADCB7;}
.st11{fill:#48B3C3;stroke:#47B3C2;stroke-width:2;stroke-miterlimit:10;}
.st12{fill:none;stroke:#47B3C2;stroke-width:10;stroke-linecap:square;stroke-linejoin:round;stroke-miterlimit:10;}
.st13{fill:#5547C2;stroke:#5547C2;stroke-width:2;stroke-miterlimit:10;}
.st14{fill:#FFFFFF;}
</style>
<g id="telefoon_00000066511502816463074380000008132361456483293328_">
<polygon id="vleugel-rechts" class="st0" points="611.4,543.8 511.6,543.8 511.6,444 "/>
<polygon id="vleugel-links" class="st0" points="64.8,543.8 164.6,543.8 164.6,444 "/>
<line id="lijn1" class="st0" x1="292.5" y1="732" x2="292.5" y2="790.9"/>
<line id="lijn2" class="st0" x1="336.5" y1="761.5" x2="336.5" y2="861.8"/>
<line id="lijn3" class="st0" x1="379.7" y1="732" x2="379.7" y2="816.4"/>
<g id="telefoon">
<path class="st1" d="M214.6,103.8h247c27.6,0,50,22.4,50,50v494c0,27.6-22.4,50-50,50h-247c-27.6,0-50-22.4-50-50v-494
C164.6,126.2,187,103.8,214.6,103.8z"/>
<path class="st2" d="M214.6,103.8h247c27.6,0,50,22.4,50,50v494c0,27.6-22.4,50-50,50h-247c-27.6,0-50-22.4-50-50v-494
C164.6,126.2,187,103.8,214.6,103.8z"/>
</g>
</g>
<g id="check_00000073696027167775002740000001036711625665836726_">
<g id="check">
<circle class="st3" cx="164.4" cy="697.8" r="56.9"/>
<path class="st4" d="M164.4,754.7c-31.4,0-56.9-25.5-56.9-56.9s25.5-56.9,56.9-56.9"/>
<path class="st4" d="M164.6,640.9c31.4,0,56.9,25.5,56.9,56.9c0,31.4-25.5,56.9-56.9,56.9"/>
<polyline class="st5" points="142.6,694.1 158.4,709.9 184.6,683.7 "/>
</g>
</g>
<g id="grafiek">
<g id="grafiek1">
<line id="Line_37_79_" class="st6" x1="654.2" y1="305.5" x2="654.2" y2="445.6"/>
</g>
<g id="grafiek2">
<line id="Line_37_78_" class="st7" x1="717.4" y1="218.8" x2="717.4" y2="445.6"/>
</g>
<g id="grafiek3">
<line id="Line_37_77_" class="st6" x1="780.6" y1="347.7" x2="780.6" y2="445.6"/>
</g>
</g>
<g id="content">
<g id="foto_kader">
<path class="st8" d="M338.1,421.8L338.1,421.8c-61,0-110.5-49.5-110.5-110.5v0c0-61,49.5-110.5,110.5-110.5h0
c61,0,110.5,49.5,110.5,110.5v0C448.6,372.3,399.1,421.8,338.1,421.8z"/>
<path class="st8" d="M448.2,421.4"/>
<path class="st8" d="M227.2,200.4"/>
<path class="st8" d="M227.2,421.4"/>
<path class="st8" d="M448.2,200.4"/>
</g>
<path id="lichaam" class="st9" d="M337.7,328.7c-37.1,0-70,18.3-90,46.4c20,28.1,52.9,46.4,90,46.4l0,0c37.1,0,70-18.3,90-46.4
C407.7,347,374.8,328.7,337.7,328.7L337.7,328.7z"/>
<g id="gezicht">
<ellipse id="Ellipse_60_18_" class="st10" cx="337.6" cy="301" rx="46.7" ry="46.7"/>
<path id="Ellipse_60_17_" class="st1" d="M348.5,328.3c0,5.9-4.8,10.8-10.8,10.8c-5.9,0-10.8-4.8-10.8-10.8H348.5z"/>
<path class="st1" d="M380.7,282.8c-7.1-16.8-23.7-28.5-43-28.5c-19.4,0-36,11.8-43,28.5H380.7z"/>
</g>
<ellipse id="home" class="st11" cx="337.7" cy="650" rx="20.1" ry="20.1"/>
<line id="tekstregel_1" class="st12" x1="287.6" y1="454.8" x2="388.6" y2="454.8"/>
<line id="tekstregel_2" class="st12" x1="315.6" y1="488.8" x2="359.6" y2="488.8"/>
<g id="button">
<g>
<rect x="287.6" y="520.8" class="st13" width="101" height="35"/>
<path class="st2" d="M388.6,555.7"/>
<path class="st2" d="M287.7,520.4"/>
<path class="st2" d="M287.7,555.7"/>
<path class="st2" d="M388.6,520.4"/>
</g>
<g>
<path class="st14" d="M308.6,543.5c-0.6,0-1.1,0-1.7-0.1c-0.6,0-1.2-0.1-1.8-0.2V533c0.5-0.1,1-0.2,1.6-0.2
c0.6,0,1.1-0.1,1.6-0.1c0.7,0,1.3,0,1.8,0.1c0.6,0.1,1,0.3,1.4,0.5c0.4,0.2,0.7,0.5,0.9,0.9c0.2,0.4,0.3,0.8,0.3,1.4
c0,0.8-0.4,1.5-1.2,2c0.7,0.3,1.1,0.6,1.4,1c0.2,0.4,0.4,0.9,0.4,1.5c0,1.1-0.4,1.9-1.2,2.5C311.4,543.2,310.2,543.5,308.6,543.5
z M307.1,536.8h1.2c0.7,0,1.2-0.1,1.6-0.3c0.3-0.2,0.5-0.5,0.5-0.9c0-0.4-0.2-0.7-0.5-0.9c-0.3-0.2-0.8-0.3-1.4-0.3
c-0.2,0-0.4,0-0.7,0c-0.2,0-0.4,0-0.6,0V536.8z M307.1,538.8v2.7c0.2,0,0.4,0,0.6,0c0.2,0,0.4,0,0.7,0c0.7,0,1.3-0.1,1.7-0.3
c0.4-0.2,0.7-0.6,0.7-1.1c0-0.5-0.2-0.8-0.5-1c-0.4-0.2-0.9-0.3-1.6-0.3H307.1z"/>
<path class="st14" d="M320.6,543.5c-0.8,0-1.4-0.1-2-0.3c-0.6-0.2-1-0.5-1.4-0.9c-0.4-0.4-0.6-0.8-0.8-1.3
c-0.2-0.5-0.3-1.1-0.3-1.7v-6.5h3v6.3c0,0.4,0,0.8,0.1,1.1c0.1,0.3,0.2,0.5,0.4,0.7c0.2,0.2,0.4,0.3,0.6,0.4
c0.2,0.1,0.5,0.1,0.8,0.1c0.6,0,1.1-0.2,1.5-0.5c0.4-0.4,0.6-1,0.6-1.8v-6.3h2v6.5c0,0.6-0.1,1.2-0.3,1.7c-0.2,0.5-0.5,1-0.8,1.3
c-0.4,0.4-0.8,0.7-1.4,0.9C322,543.4,321.4,543.5,320.6,543.5z"/>
<path class="st14" d="M336.1,532.8v2h-3v8h-2v-8h-4v-2H336.1z"/>
<path class="st14" d="M347.1,532.8v2h-4v8h-2v-8h-3v-2H347.1z"/>
<path class="st14" d="M359.1,538.1c0,0.9-0.1,1.7-0.4,2.4c-0.3,0.7-0.6,1.3-1.1,1.7c-0.5,0.5-1,0.8-1.7,1s-1.3,0.3-2.1,0.3
c-0.7,0-1.4-0.1-2-0.3c-0.6-0.2-1.2-0.6-1.7-1c-0.5-0.5-0.8-1-1.1-1.7c-0.3-0.7-0.4-1.5-0.4-2.4c0-0.9,0.1-1.7,0.4-2.4
c0.3-0.7,0.7-1.3,1.1-1.7c0.5-0.5,1-0.8,1.7-1s1.3-0.3,2-0.3c0.7,0,1.4,0.1,2,0.3s1.2,0.6,1.7,1c0.5,0.5,0.8,1,1.1,1.7
C358.9,536.4,359.1,537.2,359.1,538.1z M351.1,538.1c0,0.5,0.1,1,0.2,1.4c0.1,0.4,0.3,0.8,0.5,1.1c0.2,0.3,0.5,0.5,0.9,0.7
c0.3,0.2,0.7,0.2,1.2,0.2c0.4,0,0.8-0.1,1.2-0.2s0.6-0.4,0.9-0.7c0.2-0.3,0.4-0.7,0.5-1.1c0.1-0.4,0.2-0.9,0.2-1.4
c0-0.5-0.1-1-0.2-1.4c-0.1-0.4-0.3-0.8-0.5-1.1c-0.2-0.3-0.5-0.5-0.9-0.7c-0.3-0.2-0.7-0.2-1.2-0.2c-0.4,0-0.8,0.1-1.2,0.2
c-0.3,0.2-0.6,0.4-0.9,0.7c-0.2,0.3-0.4,0.7-0.5,1.1C351.1,537.1,351.1,537.6,351.1,538.1z"/>
<path class="st14" d="M369,543.8c-0.7-1.3-1.5-2.5-2.3-3.7c-0.8-1.2-1.7-2.4-2.6-3.5v7.2h-2v-11h2c0.3,0.3,0.7,0.8,1.2,1.3
c0.4,0.5,0.9,1.1,1.3,1.6s0.9,1.2,1.3,1.8c0.4,0.6,0.8,1.2,1.2,1.8v-6.5h2v11H369z"/>
</g>
</g>
</g>
</svg> |
|
Flaky tests detected in 58c11f6. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4291191637
|
|
Hey @tomdevisser , I was trying to debug the issue that you're reporting, but I can't seem to be able to upload SVG images as the post's featured image.
How did you manage to upload it? |
|
Hey @ciampo , I added a code snippet to the issue! |
If you refer to the PHP snippet in #48307 (comment), could you please provide more detailed instructions? For example, to which file should a developer add that code snippet? |
@ciampo Yes of course. Sorry, was replying on my phone, that's why I referred to the other ticket quickly instead of formatting it here. It is indeed this snippet. |
|
@tomdevisser I can replicate the issue you highlighted above. Using the sample SVG provided, if I add e.g.
On a separate note, I ran into some weird behaviour when dragging and dropping the SVG into the Media Library. The SVG wouldn't render correctly there. I'm assuming something funky was going on with my local env rather than the SVG itself. Just noting here in case someone else experiences the same while testing this.
I wonder if given SVG support is deliberately omitted by default, due to security issues, that makes it a little more of a niche use case. If so, perhaps it is acceptable to require the SVG images used to have defined dimensions and preservation of aspect ratio. Further tweaking and refinement of the behaviour could be considered plugin territory. While experimenting with various SVGs, I found it quite easy to create a valid SVG that wouldn't render as desired. I'm not sure we'll be able to cover all angles here. |
|
Thank you @aaronrobertshaw , I actually have the same gut instinct here. I'd be happy to go ahead and merge this PR specifically for the But I'd be cautious in assuming SVG support throughout the editor — as Aaron points out, it could either be a deliberate choice or simply lack of support. |
ciampo
force-pushed
the
feat/responsive-wrapper-SVG
branch
from
58c11f6 to
8ec8fa5
Compare
ciampo
force-pushed
the
feat/responsive-wrapper-SVG
branch
from
8ec8fa5 to
182e178
Compare
I wonder which one, and the reason behind it, but that might be something for another more general issue. So many platforms can safely handle SVGs, feels like we fall short if we don't given it's often preferred over PNG in terms of scalability, size, etc. But every step forward helps, so if this can get merged that would be great. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Alternative approach to #48307
Fixes #48184, #5477
For more context: #20892
Instead of using
useResizeObserver, use the CSSaspect-ratioprop and thewidthandmax-widthproperties to style the child element of theResponsiveWrappercomponentWhy?
This approach has a few advantages over the current approach in
trunk:SVGelements, even whennaturalWidthandnaturalHeightprops are not passedimgtag would grow without respecting its aspect ratio when the browser's viewport would be greater than the image widthuseResizeObserverhookHow?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast