-
Notifications
You must be signed in to change notification settings - Fork 21
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
base: main
Are you sure you want to change the base?
Conversation
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.
👍 💯 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.
docs/syntax/images.md
Outdated
:id: nested-carousel-example | ||
:controls: true | ||
:indicators: true | ||
:fixed-height: 350px |
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.
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?
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.
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 😆
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.
Should all be addressed now @Mpdreamz
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
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
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