Skip to content

Conversation

@XhmikosR
Copy link
Member

@XhmikosR XhmikosR commented Mar 10, 2024

@XhmikosR XhmikosR changed the title Add a render image hook docs: add a render image hook Mar 10, 2024
@XhmikosR XhmikosR marked this pull request as ready for review March 10, 2024 07:14
@XhmikosR XhmikosR marked this pull request as draft March 12, 2024 19:38
Copy link
Member

@julien-deramond julien-deramond left a 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" -}}
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

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 don't mind dropping it, I just can't recall the reason we were using it. I think it was IE, but maybe @ffoodd or @mdo remembers.

Copy link
Member

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.

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 decided to keep it for now, since it's easier to center an image if needed later.

@XhmikosR XhmikosR force-pushed the xmr/docs-render-img branch from 1d19d27 to ea0791b Compare March 13, 2024 20:12
Images are now lazyloaded, they have width and height attributes and the classes are centralized.

Only applies to Markdown images
@XhmikosR XhmikosR marked this pull request as ready for review March 15, 2024 19:14
@XhmikosR XhmikosR requested a review from mdo March 15, 2024 19:14
@XhmikosR XhmikosR merged commit 2ba7dae into main Apr 1, 2024
@XhmikosR XhmikosR deleted the xmr/docs-render-img branch April 1, 2024 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants