Skip to content

Conversation

@zihanKuang
Copy link
Contributor

@zihanKuang zihanKuang commented May 2, 2025

Notes for Reviewers

This PR fixes #522

  • Yes, I signed my commits.

Problem

The current <figure> styling does not adapt well when inline style="width:..." is present. In such cases, the figure can exceed the intended content width and introduce excessive horizontal gaps, especially on smaller screens.

Resolution

This PR updates the styling of the <figure> element in assets/scss/_styles_project.scss to:

  • Align with the behavior of markdown images rendered via layouts/_default/_markup/render-image.html

  • Ensure consistent responsiveness by applying:

    • width: 100% and max-width: 70% to figure and figure[style*="width"]
    • width: 100% to nested <img> and <figcaption>
    • object-fit: contain, display: block, and centered alignment
  • Override any inline width declarations (e.g., style="width:600px") using !important

Although <figure> is infrequently used (standard markdown image syntax is more common), it remains useful for including captions or grouped content. To support this usage, this PR also updates contributing-to-docs with usage guidance and examples.

image

zihanKuang added 2 commits May 3, 2025 00:49
Signed-off-by: Zihan Kuang <zihan_kuang@outlook.com>
Signed-off-by: Zihan Kuang <zihan_kuang@outlook.com>
@netlify
Copy link

netlify bot commented May 2, 2025

Deploy Preview for bejewelled-pegasus-b0ce81 ready!

Name Link
🔨 Latest commit 95ea8da
🔍 Latest deploy log https://app.netlify.com/sites/bejewelled-pegasus-b0ce81/deploys/68164b0ac57321000872fd04
😎 Deploy Preview https://deploy-preview-523--bejewelled-pegasus-b0ce81.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@zihanKuang zihanKuang requested review from M-DEV-1 and leecalcote May 2, 2025 23:12
@leecalcote
Copy link
Member

Here is a comparison of before and after.

before:

ScreenRecording_05-02-2025.19-09-06_1.mp4

After:

ScreenRecording_05-02-2025.19-09-37_1.mp4

I’m not positive that the after it is better in Mobile view… What do you think? The 100% with the mobile view might be a little more friendly.

@M-DEV-1
Copy link
Member

M-DEV-1 commented May 3, 2025

I agree with Lee, the images are too small to be readable, 100% width would be nice.

figure,
figure[style*="width"] {
width: 100% !important;
max-width: 70% !important;
Copy link
Member

Choose a reason for hiding this comment

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

LGTM, but do we really need both as !important?

<img src="./images/example.png" alt="Example description" style="max-width: 40vw; max-height: 60vh; display: block; margin: 1rem auto;" />
```

If you want your image to include a caption for explanation or accessibility, you can use the `<figure>` element:
Copy link
Member

Choose a reason for hiding this comment

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

these instructions are good 👍🏼

margin-bottom: 0px !important;
width: 100% !important;
height: auto !important;
object-fit: contain !important;
Copy link
Member

Choose a reason for hiding this comment

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

is !important on everything needed here?

zihanKuang added 2 commits May 3, 2025 18:51
Signed-off-by: Zihan Kuang <zihan_kuang@outlook.com>
Signed-off-by: Zihan Kuang <zihan_kuang@outlook.com>
@zihanKuang
Copy link
Contributor Author

zihanKuang commented May 3, 2025

Hi @leecalcote @M-DEV-1

Thanks for the feedback!

  1. The original looked better mainly because the review page used an inline width of 400px, which happened to be close to the screen size, so it looked like 100%. But on the performance page, the inline width is 600px, which made the image too large and not responsive.

  2. I chose 70% because that's the width we use for rendering markdown images. I wanted the images across the page to be visually consistent and aligned.

  3. 100% works better on mobile, while 70% looks better on desktop. So I added a media query to handle that, and also updated the markdown image rendering to match. Now both markdown and figure images are 100% width on screens smaller than 600px, and 70% on larger screens.

  4. As for the 600px breakpoint, I checked the Meshery and Layer5 docs. There’s a wide range of data used as reference, and I picked 600px as a reasonable cutoff based on those examples.

  5. Regarding the use of !important: since the original <figure> usage includes inline style attributes (e.g., style="width:600px"), I applied !important to override those styles without needing to manually refactor each instance.

Let me know if further adjustments are needed😎

@leecalcote
Copy link
Member

A quality PR review from, @M-DEV-1. I like it! 🙂

Sound remarks from @zihanKuang, with well-researched justification and responses, too.

@leecalcote leecalcote merged commit 267f384 into layer5io:master May 4, 2025
6 checks passed
@M-DEV-1
Copy link
Member

M-DEV-1 commented May 4, 2025

Thank you @leecalcote,

@zihanKuang thanks for justifying the usage 👌

@zihanKuang zihanKuang deleted the fix-image-clean branch May 4, 2025 15:56
leecalcote added a commit that referenced this pull request Jun 10, 2025
Fix: Enforce consistent, responsive `<figure>` styling to match markdown image behavior
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix responsiveness issue on /kanvas/advanced/performance/ page (mobile)

3 participants