Skip to content

Adding support to the docs for an image carousel #1396

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

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

kgeller
Copy link

@kgeller kgeller commented Jun 18, 2025

Summary

Adding support to the docs-builder for an image carousel.

We want to use this to display the dashboard screenshots in the docs similarly to what we do already in Kibana.

The only setting for the carousel is an optional setting for fixed-height. The options are: small, medium, auto where auto simply defaults to whatever the image size is, small is 350px and medium is 750px. These sizes are just what I thought looked ok, I can change them to whatever people prefer.

How it works

We add the following directive to the docs

::::{carousel}
:fixed-height: medium
:::{image} https://epr.elastic.co/package/abnormal_security/1.8.0/img/abnormal_security-mailbox_not_analyzed_overview.png
:alt: First image description
:title: First image title
:::
:::{image} https://epr.elastic.co/package/abnormal_security/1.8.0/img/abnormal_security-ai_security_mailbox_overview.png
:alt: Second image description
:title: Second image title
:::
:::{image} https://epr.elastic.co/package/abnormal_security/1.8.0/img/abnormal_security-audit_overview.png
:alt: Third image description
:title: Third image title
:::
::::

And this is what gets generated

Screen.Recording.2025-06-18.at.4.00.13.PM.mov

Also added to the documentation of what is supported

Screenshot 2025-06-20 at 3 00 47 PM

Notes

I wanted to build upon the existing image directive just so that I could get the existing functionality there around popping open an image in a new tab.

I added the fixed-height setting as because 1) not all of these screenshots are the same size, and I didn't want the buttons to click left / right to be jumping around the screen and 2) some of them are quite large and could be quite overbearing to the page.

Relates https://github.com/elastic/integration-experience/issues/83

@kgeller kgeller marked this pull request as ready for review June 18, 2025 20:13
@kgeller kgeller requested a review from a team as a code owner June 18, 2025 20:13
Copy link
Member

@Mpdreamz Mpdreamz left a comment

Choose a reason for hiding this comment

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

👍 💯 Great PR! What a treat to get a new directive complete with with diagnostics and ❤️ the reuse of the image directive.

I have a few small nitpicks on the options to the carrousel directive.

Comment on lines 125 to 128
:id: nested-carousel-example
:controls: true
:indicators: true
:fixed-height: 350px
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this id'less? The discovery could pass the HtmlElement to the ImageCarousel constructur to contextualize everything?

We try to have as few options as possible in order to promote uniformity in the docs.

I'd reckon we always want controls and indicators so we can remove the options for them. Happy to hear other thoughts though!

Is fixed-height optional?

Should we support auto to pick the height of the biggest image automatically? Should that be the default.

Do we want folks to specify other sizes ? If resizing is fine maybe we should give a limited set of options e,g

fixed-height: auto | small | medium and we uniformly decide what small and medium is?

Copy link
Author

Choose a reason for hiding this comment

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

Can we make this id'less? The discovery could pass the HtmlElement to the ImageCarousel constructur to contextualize everything?

Will update!

I'd reckon we always want controls and indicators so we can remove the options for them. Happy to hear other thoughts though!

Also fair, and I can remove those. I debated internally and left them in as I didn't realize we tried to have as few options as possible.

Is fixed-height optional?

It should be, but I will double check when I address the below feedback.

Should we support auto to pick the height of the biggest image automatically? Should that be the default.
Do we want folks to specify other sizes ? If resizing is fine maybe we should give a limited set of options e,g fixed-height: auto | small | medium and we uniformly decide what small and medium is?

This also makes sense to me and I can update to that. I just wanted a way to be able to not have the images not dominate the screen since some of them will by default 😆

Copy link
Author

Choose a reason for hiding this comment

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

Should all be addressed now @Mpdreamz

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants