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(imsize): rework image styling to get more control #549

Merged
merged 5 commits into from
Nov 7, 2024

Conversation

St1ggy
Copy link
Contributor

@St1ggy St1ggy commented Nov 2, 2024

These changes are necessary in order to gain more control over the appearance of the images in cases where we need to maintain the proportions when resizing the container.
Now this is a bit bugged - when the height exceeds the expected one, we see empty space around the image from above and below

@d3m1d0v
Copy link
Member

d3m1d0v commented Nov 2, 2024

plz, add more tests:

  • with only height in percent
  • with width and height in percent

@St1ggy
Copy link
Contributor Author

St1ggy commented Nov 2, 2024

Problem: If we have an image with predefined dimensions in the markup (height and width), then when it is displayed in a container, it is assumed to be of those sizes. However, the CSS already sets max-width: 100%, limiting the image to the width of the container. This causes a problem of "transparent frames" at the top and bottom when the image's width decreases along with the width of the container. The height remains constant, not depending on the proportions of the image.

The proposed solution in the pull request allows us to control how the image displays according to its proportions. When we resize the container, the image's width will decrease as before, but its height will now depend on an aspect-ratio parameter instead of the height attribute. This allows the image to shrink and expand proportionally, without the bug of empty frames at the top and bottom.

P.S. I also added some tests with percent sizes

Copy link
Contributor

@shevnv shevnv left a comment

Choose a reason for hiding this comment

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

I'm not sure that right proportions for image are more important than height+width from markup.

@@ -206,6 +206,27 @@ export const imageWithSize = (md: MarkdownIt): ParserInline.RuleInline => {
if (height !== '') {
token.attrs.push([ImsizeAttr.Height, height]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we still need height and width attributes?

Copy link
Contributor Author

@St1ggy St1ggy Nov 2, 2024

Choose a reason for hiding this comment

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

const width = Number(imageToken.attrGet('width'));
const height = Number(imageToken.attrGet('height'));

Alas, that's why we still need it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also noticed complaints about incorrect image appearance when the height was not proportional. However, I did not see any requests for a stretched image or an image with extra frames.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The proportions of the image are calculated based on the height and width specified in the markup. If the container is larger than these dimensions, the image will maintain its original height and width. If the width of the container is less than the width of the image, the image's width will adjust to 100% of the container's width and the height will be adjusted proportionally based on the original height and width values from the markup.

@St1ggy St1ggy requested a review from shevnv November 5, 2024 10:23
@St1ggy St1ggy force-pushed the rework-imsize-styling branch 2 times, most recently from 3cd0e21 to 3d1ab7a Compare November 6, 2024 10:23
@St1ggy St1ggy force-pushed the rework-imsize-styling branch 2 times, most recently from 6d3b490 to 09f1cbf Compare November 6, 2024 12:53
if (height !== '') {
if (width !== '' && !heightWithPercent && !widthWithPercent) {
style += `aspect-ratio: ${width} / ${height};height: auto;`;
forcedSanitizeWhiteList['aspect-ratio'] = true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to enforce the 'aspect-ratio' rule for inline CSS sanitizing. Otherwise, this rule may be ignored and some artifacts may occur.

@St1ggy St1ggy force-pushed the rework-imsize-styling branch 2 times, most recently from 6f8a7ae to 648f539 Compare November 6, 2024 16:33
export default function sanitize(
html: string,
options?: SanitizeOptions,
forcedSanitizeCssWhiteList?: CssWhiteList,
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. I suggest replacing it with an interface that has an additional parameter for uniformity and future expansion of functionality.
    html: string,
    options?: SanitizeOptions,
    additionalOptions?: SanitizeOptions,
  1. Then it will be:
    if (forcedSanitizeCssWhiteList) {
        sanitizeOptions.cssWhiteList = {
            ...sanitizeOptions.cssWhiteList,
            ...additionalOptions.cssWhiteList,
        };
    }

@@ -5,7 +5,11 @@ import type Token from 'markdown-it/lib/token';
import {ImsizeAttr} from './const';
import {parseImageSize} from './helpers';

export const imageWithSize = (md: MarkdownIt): ParserInline.RuleInline => {
export type ImsizeOptions = {
inlineSizeStyling?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe better is enableInlineStyling

@St1ggy St1ggy force-pushed the rework-imsize-styling branch from 59709e4 to 3aa9433 Compare November 7, 2024 13:35
@St1ggy St1ggy force-pushed the rework-imsize-styling branch from 3aa9433 to d6ce8fb Compare November 7, 2024 14:58
@St1ggy St1ggy merged commit 3036e6d into master Nov 7, 2024
1 check passed
@St1ggy St1ggy deleted the rework-imsize-styling branch November 7, 2024 16:06
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.

4 participants