Skip to content

Conversation

@vishalvivekm
Copy link
Contributor

@vishalvivekm vishalvivekm commented Jul 4, 2025

Notes for Reviewers

This PR fixes #
https://deploy-preview-628--bejewelled-pegasus-b0ce81.netlify.app/learning-paths/

Signed commits

  • Yes, I signed my commits.

vishalvivekm added 3 commits July 4, 2025 06:36
@netlify
Copy link

netlify bot commented Jul 4, 2025

Deploy Preview for bejewelled-pegasus-b0ce81 ready!

Name Link
🔨 Latest commit 0252c3b
🔍 Latest deploy log https://app.netlify.com/projects/bejewelled-pegasus-b0ce81/deploys/686ffbdc09899000088794ca
😎 Deploy Preview https://deploy-preview-628--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 project configuration.

vishalvivekm added 2 commits July 4, 2025 06:47
Signed-off-by: vishalvivekm <[email protected]>
{{ $title := .Get "title" | default "" }}

<div class="image-container" style="text-align: {{ $align }}; margin: 1rem 0;">
<img src="{{ $src | safeURL }}" alt="{{ $alt }}" {{ with $title }}title="{{ . }}"{{ end }} style="width: {{ $width }}; max-width: 100%; height: auto; border-radius: 0.5rem; box-shadow: 0 0 0.5rem rgba(0, 179, 159, 0.4);" data-modal="true" onclick="openModal(this)" />
Copy link
Contributor

@zihanKuang zihanKuang Jul 4, 2025

Choose a reason for hiding this comment

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

Hi @vishalvivekm,
Great work! I learned a lot from it.✍️

I noticed that the images on these new pages look a bit different from those in the rest of our docs. It seems the inline styles in image.html are overriding the global CSS, causing inconsistent sizing (they appear to be 100% width).

Perhaps we could refactor layouts/shortcodes/image.html to produce a similar HTML structure to our layouts/_default/_markup/render-image.html file. This would ensure that no matter which method is used to add an image, the styling remains the same across the entire site.

What do you think?👀

image

For comparison:

Standard Doc Image New Learning Path Image
Normal doc image Learning path image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted inline styles 👍

Signed-off-by: vishalvivekm <[email protected]>
Copy link
Contributor

@zihanKuang zihanKuang left a comment

Choose a reason for hiding this comment

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

LGTM😎

Copy link
Contributor

@zihanKuang zihanKuang left a comment

Choose a reason for hiding this comment

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

Hi @vishalvivekm
I noticed a couple of minor rendering issues. I did some experimenting and I think I've found the causes:

  1. alert Shortcode Rendering Issue: On pages like interpreting-meshery-designs, the alert shortcodes aren't rendering correctly. The content inside isn't parsed as Markdown (e.g., **text** appears with raw asterisks) and the component is missing its proper CSS styling.
    I found that this only happens when the alert is nested inside the {{< chapterstyle >}} shortcode. After I removed the chapterstyle wrapper from the top and bottom of the file, the alert rendered perfectly. But I'm not sure if this has any other purpose.🤔

  2. Bulleted List Formatting: In the screenshot I'm looking at from reviewing-designs, some bulleted lists (* ...) are broken into separate paragraphs.
    An extra blank line between list items will break the <ul> list.

Hope this feedback is helpful. Amazing work on this huge migration!

1751689526474 1751689526485

…shery/reviewing-designs.md

Co-authored-by: Zihan Kuang <[email protected]>
Signed-off-by: Aabid Sofi <[email protected]>
@aabidsofi19 aabidsofi19 merged commit d5a7b90 into master Jul 10, 2025
5 of 6 checks passed
@aabidsofi19 aabidsofi19 deleted the port-learning-path branch July 10, 2025 17:44
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.

4 participants