-
-
Notifications
You must be signed in to change notification settings - Fork 173
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
Missing width/height attribute for images when added via (image: ) / kirbytext #5064
Comments
I know that the
|
The kirbytag already supports the width and height attributes, but of course, it would be a bit painful to have to add these manually. On the other hand, maybe not everyone wants to have these attributes added automatically. Therefore, some attribute that lets the user decide easily would probably make sense? |
I had completely missed that browsers now apparently support getting the aspect ratio from the attributes even when I wonder if there is any disadvantage of adding the attributes always and by default. There is the breaking change that relative units of the Because if there is no significant disadvantage, I'd vote to add the |
As Lukas said, I would also vote in favour of rendering the |
@lukasbestle I don't understand this one, can you explain? Might just be me after a long day. Cause I would also think by default could work. |
If you set the native image dimensions with the img {
width: 100%;
} The width will be overridden but the height won't be. So the browser will use the absolute height from the attribute but scale the width dynamically. Thus the aspect ratio will be incorrect. If you don't use the attributes (current behavior of the Kirbytag), the browser will keep the aspect ratio and scale the height dynamically as well. But once you use the attributes, you need to set |
Ah now got it, thanks! |
It is also one of my frequent uses. |
I find this discussion really helpful and interesting to read along, thanks for considering my report! My concern is that turning this on by default in a minor update to Kirby might put people off who bought a template or others who sell templates and receive unexpected support requests. I‘ve been giving this further thought, and I think I have found a solution to the breaking-change-problem. I think going for a soft rollout as a config option in Kirby 3.X, and turning it on by default in Kirby 4 could be an option? I think introducing two keywords for the return [
'kirbytext' => [
'image' => [
'height' => intrinsicHeight,
'width' => intrinsicWidth,
]
]
]; Adapted from: https://getkirby.com/docs/reference/system/options/kirbytext#image Edit: I changed my proposal to use camel-case. I think Kirby should camel-case such keywords in general, but I believe that is a different discussion. It would make them easier to read and it would make them more accessible to people with limited eyesight. |
3.x releases are major releases (in the versioning scheme We could still introduce the new behavior with an option first (similar to what you proposed). This would allow devs to slowly migrate their sites and themes. However the syntax you suggested won't work. Like that they would be constants and would need to be all-caps and namespaced in a class. We could make them strings though. |
I noticed a second complication: The We could still support automatic width and height attributes for local files though. For external files it would be on the editor to add them manually to the tag. Suggestion on a way forward:
|
Description
When Kirby renders a field containing the kirbytext flavor of markdown into HTML, it does not add
width
andheight
attributes to the<img />
element. Besides their obvious functionality, these two fields inform the browser about an image‘s aspect ratio. Adding them will improve the experience for site visitors on bad/slow connections, and users of thekirbytext()
-method can currently not add this themselves easily. I did not test themarkdown()
-method. If the lack ofwidth
/height
attributes is considered a bug, themarkdown()
-method may need to be updated as well to be consistent.I understand this might constitute a breaking change for some themes, as there will be the need to update some theme‘s CSS to include a
height
-attribute. Still, I think the improvement is worth considering. There is information about this in this older video from 2019: https://youtu.be/4-d_SoCHeWEExpected behavior
Any
(image: )
element contained in a Field should be rendered into HTML that includes thewidth
andheight
attributes for that image. Considering images currently render afigure
-element containing animg
-element, I would expect this result:Alternative 1
I considered suggesting this alternative: Add
style: "aspect-ratio: [width] / [height]"
to allimg
-elements. Example:However, this would give the aspect-ratio the highest specificity and likely break more than adding the
width
/height
attributes.Alternative 2
Do nothing. I considered if adding
width
/height
attributes would do more harm than good. I do not know for sure. As adding them is a best-practice to avoid reflows after the initial render of the page, I think the fact that they are missing is a bug.To reproduce
(image: )
to a kirbytext-field in a page‘s TXT fileexample.txt
Your setup
Kirby Version
3.9.1
Your system (please complete the following information)
The text was updated successfully, but these errors were encountered: