-
-
Notifications
You must be signed in to change notification settings - Fork 79.2k
docs: add a render image hook #39768
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
Conversation
julien-deramond
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR looks good to me, just a minor comment regarding the .d-block class that doesn't seem mandatory.
| {{- /* This shouldn't be needed but we have a weird folder structure with version included... */ -}} | ||
| {{- $src := urls.JoinPath "/docs" site.Params.docs_version $originalSrc -}} | ||
| {{- $config := imageConfig $localImgPath -}} | ||
| {{- $classes := "d-block img-fluid" -}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you found any use cases needing to add this .d-block class? Doesn't seem to be useful in the current usage within the guides.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are using d-block in blog and I'd swear at some point we needed d-block for some browser to work correctly, but maybe it was IE and SVG files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, no problem. I don't mind keeping it for consistency or safety if you think it can be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My guess it allowed to drop bottom margin that came with inline and inline-block images in some cases, especially with img-fluid.
Could also relate to how images behave as flex children.
Not quite sure though 🤷 Would need some history digging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to keep it for now, since it's easier to center an image if needed later.
1d19d27 to
ea0791b
Compare
Images are now lazyloaded, they have width and height attributes and the classes are centralized. Only applies to Markdown images
Images are now lazyloaded, they have width and height attributes and the classes are centralized.
Only applies to Markdown images
Previews: