Skip to content

fix: markdown rendering for custom types#483

Open
sharanyavinod wants to merge 7 commits into
mainfrom
ew-md
Open

fix: markdown rendering for custom types#483
sharanyavinod wants to merge 7 commits into
mainfrom
ew-md

Conversation

@sharanyavinod
Copy link
Copy Markdown
Contributor

Preview at https://da.live/canvas?nx=ew-md&nxver=2#/exp-workspace/frescopa/index

@aem-code-sync
Copy link
Copy Markdown

aem-code-sync Bot commented May 29, 2026

Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch.
In case there are problems, just click the checkbox below to rerun the respective action.

  • Re-sync branch
Commits

Comment thread nx2/blocks/chat/renderers.js
Comment thread nx2/blocks/chat/chat.css Outdated
position: absolute;
inset: 0;
background-color: var(--s2-gray-25);
mask-image: url("/nx2/img/icons/S2_Icon_Checkmark_20_N.svg");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

mask-image: url("/nx2/img/icons/S2_Icon_Checkmark_20_N.svg") is a hardcoded absolute asset path. breaks if the asset structure ever moves. we have flagged this before (see other icon usage with loadHrefSvg). @hannessolo what was the decision here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, we should use <svg><use> wherever possible. The icons shouldn't be added to the code, but referenced directly from da.live, eg. https://da.live/img/icons/s2-icon-checkmark-20-n.svg

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is pure styling, so didnt want to add an svg. I'll update to have the live url then - I saw a couple other similar usages, will adapt those in parallel as well.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, that's why "wherever possible" - I think here your approach might make more sense. But I'm not sure, will dark mode work with mask-image?
In either case we should use the URL from da.live instead of adding the svg to the code.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, currentcolor on the background will inherit the parent color. I'll adapt to use the url -> for the others I think we might need to first add the icons to da live

Comment thread nx2/blocks/chat/chat.css Outdated
width: 14px;
height: 14px;
flex-shrink: 0;
margin-top: 4px;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we're using var(--s2-spacing-75) right next to these. do spacing tokens cover 4px and 6px? if so, let's use them too.

Comment thread nx2/blocks/chat/renderers.js Outdated
@hannessolo
Copy link
Copy Markdown
Contributor

Nice, looks much better!

I'm fine with the current implementation for now, but as an improvement we should find a way to already render the correct style before the closing tag appears.

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.

3 participants