Skip to content
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.

Sign up for GitHub

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

Fix: Set the fit-content width for images that are not .svg #66643

Merged
merged 1 commit into from
Nov 4, 2024
Merged

Fix: Set the fit-content width for images that are not .svg #66643

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion packages/block-library/src/image/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,10 @@
max-width: 100%;
vertical-align: bottom;
box-sizing: border-box;
width: fit-content;

&:not([src$=".svg"]) {
Copy link
Member

@kevin940726 kevin940726 Nov 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we be certain that svg files always have .svg at the end of their sources in WP?

Copy link
Member Author

@juanfra juanfra Nov 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kevin940726 I believe in the context of the image block, yes. I was researching, and the other option is the .svgz extension, which is for compressed SVGs. I never saw any, but we could add it, to make it more robust and cover all cases.

Suggested change
&:not([src$=".svg"]) {
&:not([src$=".svg"]):not([src$=".svgz"]) {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm mostly concerned that SVG files don't have to end with the .svg postfix in the URLs. Browsers look for the content types, not the file extensions. I'm not sure if this applies to this case though!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's not certain but highly likely. So if this fixes for 80% of sites then it's an improvement.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm mostly concerned that SVG files don't have to end with the .svg postfix in the URLs. Browsers look for the content types, not the file extensions. I'm not sure if this applies to this case though!

This is true, but I believe it doesn't apply to this case, where the SVG is uploaded via UI and inserted via image block. If I'm not mistaken, the media library/image block should insert images with their file extensions.

The problem would be if a block themer, includes an image block on some pattern or template, where the image src uses data:image/..., and if the image doesn't have dimensions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all, I agree! I think this is a good improvement even though I have some minor concerns. I think we can merge this and iterate if necessary.

If I'm not mistaken, the media library/image block should insert images with their file extensions.

Would someone be able to insert an SVG file via an external URL? I'm not too familiar with the media upload module, but could someone upload an SVG file without the extension? That is, does the code look for the MIME type or the file extension?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would someone be able to insert an SVG file via an external URL? I'm not too familiar with the media upload module, but could someone upload an SVG file without the extension?

As far as I know, not via UI using the medialibrary.

width: fit-content;
}

@media (prefers-reduced-motion: no-preference) {
&.hide {
Expand Down
Loading