Skip to content
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

feat: Add image captions support #586

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

osulyanov
Copy link

@osulyanov osulyanov commented Dec 16, 2024

Add support for image captions using figcaption tag

Description

Added support for image captions using HTML's semantic figure and figcaption tags.
This enhances image context and improves document accessibility by providing visual relationships between images and their captions.

Features

  • New caption syntax with two options:
    1. Use the title as caption: ![Alt text](image.jpg "Caption text" =100x100){ caption }
    2. Explicit caption: ![Alt text](image.jpg "Title" =100x100){ caption="Custom caption text" }
  • Outputs semantic HTML using <figure> and <figcaption> tags
  • Maintains compatibility with existing image features (eg. SVG inlining)

Example

Input:

![Alt text](image.jpg "Default caption text" =100x100){ caption }
![Alt text](image.jpg "Title" =100x100){ caption="Specified caption text" }

Output:

<figure>
    <img src="image.jpg" alt="Alt text" title="Default caption text" width="100" height="100">
    <figcaption>Default caption text</figcaption>
</figure>

<figure>
    <img src="image.jpg" alt="Alt text" title="Title" width="100" height="100">
    <figcaption>Specified caption text</figcaption>
</figure>

Documentation

Corresponding documentation PR: diplodoc-platform/docs#80

Technical Changes

  • Added new markdown-it inline rule for parsing caption syntax
  • Implemented token transformation for figure/figcaption wrapping
  • Fixed image path replacement to handle multiple occurrences
  • Improved loop handling for patched images

Testing

  • Manual testing with various image caption configurations
  • Verified compatibility with existing image features
  • Tested multiple images in a single document

Closes DOCSTOOLS-3428

Related issue: #514

@osulyanov osulyanov changed the title feat: Add image capttions support feat: Add image captions support Dec 16, 2024

while (j < childrenTokens.length) {
Copy link
Member

Choose a reason for hiding this comment

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

Replace default token image with image_with_caption can be wery destructive for external plugins, which searches for image token only.

I propose to implement this in other way:

  • Check what image token has caption attr.
  • Then generate new token image_caption after this one.
  • Add renderer for image_caption which will generate all required a11y compatible html.
  • Convert caption attr on image to some class attr image.attrSet('class', image.attrGet('class') + ' yfm-image-with-caption')
  • Add some styles to main css.

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense, I got rid of image_with_caption token

Copy link
Author

Choose a reason for hiding this comment

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

Wdyt, does the current solution work or potentially could have a negative impact on other image-related plugins?
What about the CSS? Do I need to align it to the left?
Screenshot 2024-12-18 at 19 01 38

@3y3
Copy link
Member

3y3 commented Dec 16, 2024

Thank you, @osulyanov, for your contribution. The PR description was great, making it easy to understand all the changes.

I've added some comments to help reduce the impact of the change.

Check out this comment, where I discuss the issue of backward compatibility.

    Don't use separate image_capture tag
    that potentially can break other image-related plugings
@osulyanov
Copy link
Author

@3y3 Thanks for the fast review and valuable and well-described comments. I made the changes to align with them, please have a look if it works.

@osulyanov osulyanov requested a review from 3y3 December 18, 2024 15:49
@3y3
Copy link
Member

3y3 commented Dec 20, 2024

This looks like approvable PR. I will merge it at the end of week.
If you want to improve it even more, you can add some tests.

I'm also not sure what this four lines are required, but I need to test it later
https://github.com/diplodoc-platform/transform/pull/586/files#diff-e21601ef2d076218747acc804a5a8a16f35a4eb71ef0b73e6db53d005b074869R204

    Renderer rules are not needed because markdown-it already has default HTML renderers for basic HTML tags
@osulyanov
Copy link
Author

You were right, markdown-it already has default HTML renderers for basic HTML tags so these renderer rules were redundant, so I removed them.

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

Successfully merging this pull request may close these issues.

2 participants