-
Notifications
You must be signed in to change notification settings - Fork 88
Feature/enhanced markdown rendering support #732
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
base: dev
Are you sure you want to change the base?
Feature/enhanced markdown rendering support #732
Conversation
- Add comprehensive footnotes support in KotlinRenderer - Footnote references rendered as superscript links - Footnote definitions rendered as block elements - Proper ID-based linking between references and definitions - Enhance task list support in MarkdownHandlers - Automatic detection of task list patterns in all text contexts - Support for - [x] (checked) and - [ ] (unchecked) syntax - FontAwesome icons when Silk is available, HTML checkboxes fallback - Works in table cells and other complex structures - Add footnotes feature configuration in MarkdownFeatures - Configurable footnotes support (enabled by default) - Integration with CommonMark FootnotesExtension - Update playground with test examples for new features Resolves issues with markdown rendering inconsistencies and adds support for GitHub Flavored Markdown footnotes feature.
…ist renderingtency and features.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really appreciate you getting your hands dirty and jumping into some of the uglier code in the Kobweb project :)
Taking a look at your PR, I think the biggest issue overall is some handler code ending up inside renderer code, and some parsing logic ending up inside handlers.
It should go: parser (focused only on markdown) -> renderer (focused mostly on visiting nodes) -> handlers (focused on providing Kotlin/JS code), something like that. (It's been a while, my memory is a bit hazy, but if you see some code that seems to contradict this, feel free to call attention to it)
I haven't looked too deeply at the proposed code yet to see if I could suggest a better way. I think for now I'll just leave comments mentioning my concerns and, if you can't see a way to resolve them (say, my bad code won't allow it), let me know and we can go back and forth.
For now, I would recommend simplifying this PR to just strikethrough, because that seems like a relatively simple extension. And we can tackle the footnote stuff / comprehensive tests later.
Also, I'll do the next release without these markdown changes since they're a bit more involved than I thought; we can get them ready for the release after that. That way, there's no need to feel any pressure to rush.
playground/site/src/jsMain/resources/markdown/documents/unified_test.md
Outdated
Show resolved
Hide resolved
playground/site/src/jsMain/resources/markdown/documents/unified_test.md
Outdated
Show resolved
Hide resolved
...s/extensions/markdown/src/main/kotlin/com/varabyte/kobwebx/gradle/markdown/KotlinRenderer.kt
Outdated
Show resolved
Hide resolved
...extensions/markdown/src/main/kotlin/com/varabyte/kobwebx/gradle/markdown/MarkdownFeatures.kt
Outdated
Show resolved
Hide resolved
...extensions/markdown/src/main/kotlin/com/varabyte/kobwebx/gradle/markdown/MarkdownFeatures.kt
Outdated
Show resolved
Hide resolved
...s/markdown/src/main/kotlin/com/varabyte/kobwebx/gradle/markdown/handlers/MarkdownHandlers.kt
Outdated
Show resolved
Hide resolved
...s/extensions/markdown/src/main/kotlin/com/varabyte/kobwebx/gradle/markdown/KotlinRenderer.kt
Outdated
Show resolved
Hide resolved
...s/extensions/markdown/src/main/kotlin/com/varabyte/kobwebx/gradle/markdown/KotlinRenderer.kt
Outdated
Show resolved
Hide resolved
...s/extensions/markdown/src/main/kotlin/com/varabyte/kobwebx/gradle/markdown/KotlinRenderer.kt
Outdated
Show resolved
Hide resolved
playground/site/src/jsMain/resources/markdown/documents/unified_test.md
Outdated
Show resolved
Hide resolved
- Add comprehensive footnotes support in KotlinRenderer - Footnote references rendered as superscript links - Footnote definitions rendered as block elements - Proper ID-based linking between references and definitions - Enhance task list support in MarkdownHandlers - Automatic detection of task list patterns in all text contexts - Support for - [x] (checked) and - [ ] (unchecked) syntax - FontAwesome icons when Silk is available, HTML checkboxes fallback - Works in table cells and other complex structures - Add footnotes feature configuration in MarkdownFeatures - Configurable footnotes support (enabled by default) - Integration with CommonMark FootnotesExtension - Update playground with test examples for new features Resolves issues with markdown rendering inconsistencies and adds support for GitHub Flavored Markdown footnotes feature.
…ist renderingtency and features.
…sed layout property
- Consolidate `footnotes` and `strikethrough` properties for clarity - Adjust initialization sequence for extensions to ensure proper loading and maintain code readability
# Conflicts: # playground/site/src/jsMain/resources/markdown/Markdown.md # tools/gradle-plugins/extensions/markdown/src/main/kotlin/com/varabyte/kobwebx/gradle/markdown/KotlinRenderer.kt # tools/gradle-plugins/extensions/markdown/src/main/kotlin/com/varabyte/kobwebx/gradle/markdown/MarkdownFeatures.kt # tools/gradle-plugins/extensions/markdown/src/main/kotlin/com/varabyte/kobwebx/gradle/markdown/handlers/MarkdownHandlers.kt
|
let me know if you have any feedback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't run your code yet, I will after the next round of comments. But this definitely looks way cleaner than the first approach, thanks for taking the time to refactor.
| @@ -1,5 +1,14 @@ | |||
| @file:Suppress("LeakingThis") // Following official Gradle guidance | |||
|
|
|||
| /** | |||
There was a problem hiding this 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 why you added this block. People can use git history for this, and meanwhile, I don't think future devs will maintain it.
| is TableBody -> visit(customNode) | ||
| is TableRow -> visit(customNode) | ||
| is TableCell -> visit(customNode) | ||
| is FootnoteReference -> visit(customNode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Footnote should be alphabetical
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are there two markdown unified_test files?
...s/extensions/markdown/src/main/kotlin/com/varabyte/kobwebx/gradle/markdown/KotlinRenderer.kt
Outdated
Show resolved
Hide resolved
...s/extensions/markdown/src/main/kotlin/com/varabyte/kobwebx/gradle/markdown/KotlinRenderer.kt
Outdated
Show resolved
Hide resolved
| val isChecked = marker.isChecked | ||
| if (useSilk.get()) { | ||
| val iconCode = if (isChecked) { | ||
| "com.varabyte.kobweb.silk.components.icons.fa.FaSquareCheck" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, we should not rely on FA (or any external) icons -- a user definitely does not have to have them included in their project.
Instead, we should be using SVG icons that are included with Silk.
Go to the Widgets table of the playground example and look at the Checklist widget.
The checkbox checkmark is driven by the CheckIcon SVG icon (see here).
Can you use a Silk Checkbox widget here instead? That would be easiest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when I use SILK
taskListItemMarker.convention { marker ->
"com.varabyte.kobweb.silk.components.forms.Checkbox(checked = $marker.isChecked, enabled = false, onCheckedChange = {})"
}
the checkbox is as in the below image and its generated content :
<li>
<label class="silk-checkbox kobweb-row kobweb-arrange-start kobweb-align-center-vert silk-disabled silk-checkbox-size_md" aria-disabled="true" tabindex="-1">
<input class="silk-input silk-input-checkbox silk-input-size_md" type="checkbox" aria-disabled="true" disabled="" spellcheck="false">
<div class="silk-checkbox-icon-container silk-checkbox-icon-container-checked kobweb-box kobweb-align-center">
<div class="silk-checkbox-icon kobweb-box kobweb-align-center">
<svg style="overflow: visible;" width="1em" viewBox="0 0 24 20" stroke="currentColor" fill="none" stroke-width="4">
<polyline points="3,12 9,18 21,2"></polyline>
</svg>
</div>
</div>
</label>
Completed task
</li>
While when I do use KOBWEB_DOM
taskListItemMarker.convention { marker ->
"$KOBWEB_DOM.GenericTag(\"input\", \"type=\\\"checkbox\\\" ${if (isChecked) "checked " else ""}disabled\")"
}
it gets rendered correctly, see image below and its generated content :
<li><input type="checkbox" checked="" disabled="">Completed task</li>
I'm not sure why SILK isn't displaying things right, or if this was intended. Let me know if you would still recommend SILK only or maybe KOBWEB_DOM (Compose).
I initially thought doing this would be safe but I don't have a strong rational for it
taskListItemMarker.convention { marker ->
val isChecked = marker.isChecked
if (useSilk.get())
"$SILK.forms.Checkbox(checked = $isChecked, enabled = false, onCheckedChange = {})"
else
"$KOBWEB_DOM.GenericTag(\"input\", \"type=\\\"checkbox\\\" ${if (isChecked) "checked " else ""}disabled\")"
}
...s/markdown/src/main/kotlin/com/varabyte/kobwebx/gradle/markdown/handlers/MarkdownHandlers.kt
Outdated
Show resolved
Hide resolved
|
|
||
| footnoteDefinition.convention { definition -> | ||
| val label = definition.label | ||
| "$KOBWEB_DOM.GenericTag(\"div\", \"class=\\\"footnote-item\\\" id=\\\"fn-$label\\\"\")" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you using a generictag div here? You should just be able to use Compose HTML's Div directly.
...s/markdown/src/main/kotlin/com/varabyte/kobwebx/gradle/markdown/handlers/MarkdownHandlers.kt
Outdated
Show resolved
Hide resolved
...s/markdown/src/main/kotlin/com/varabyte/kobwebx/gradle/markdown/handlers/MarkdownHandlers.kt
Outdated
Show resolved
Hide resolved
…t' into feature/enhanced-markdown-support
…items and containers
5c34e2e to
e726ac5
Compare
Description
Enhanced markdown support with comprehensive footnotes and improved task lists functionality.
Changes Made
Problem Solved
This PR addresses inconsistencies in markdown rendering, specifically:
Testing
Type of Change
Fixes: Enhanced markdown rendering capabilities