-
Notifications
You must be signed in to change notification settings - Fork 45
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
Add tables extension #339
Add tables extension #339
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
public final class org/jetbrains/jewel/markdown/extensions/tables/GitHubTableBlockRenderer : org/jetbrains/jewel/markdown/extensions/MarkdownBlockRendererExtension { | ||
public static final field $stable I | ||
public fun <init> (Lorg/jetbrains/jewel/markdown/extensions/tables/TableStyling;Lorg/jetbrains/jewel/markdown/rendering/MarkdownStyling;)V | ||
public fun canRender (Lorg/jetbrains/jewel/markdown/MarkdownBlock$CustomBlock;)Z | ||
public fun render (Lorg/jetbrains/jewel/markdown/MarkdownBlock$CustomBlock;Lorg/jetbrains/jewel/markdown/rendering/MarkdownBlockRenderer;Lorg/jetbrains/jewel/markdown/rendering/InlineMarkdownRenderer;Landroidx/compose/runtime/Composer;I)V | ||
} | ||
|
||
public final class org/jetbrains/jewel/markdown/extensions/tables/GitHubTableProcessorExtension : org/jetbrains/jewel/markdown/extensions/MarkdownProcessorExtension { | ||
public static final field $stable I | ||
public static final field INSTANCE Lorg/jetbrains/jewel/markdown/extensions/tables/GitHubTableProcessorExtension; | ||
public fun getParserExtension ()Lorg/commonmark/parser/Parser$ParserExtension; | ||
public fun getProcessorExtension ()Lorg/jetbrains/jewel/markdown/extensions/MarkdownBlockProcessorExtension; | ||
public fun getTextRendererExtension ()Lorg/commonmark/renderer/text/TextContentRenderer$TextContentRendererExtension; | ||
} | ||
|
||
public final class org/jetbrains/jewel/markdown/extensions/tables/GitHubTableRendererExtension : org/jetbrains/jewel/markdown/extensions/MarkdownRendererExtension { | ||
public static final field $stable I | ||
public fun <init> (Lorg/jetbrains/jewel/markdown/extensions/tables/TableStyling;Lorg/jetbrains/jewel/markdown/rendering/MarkdownStyling;)V | ||
public fun getBlockRenderer ()Lorg/jetbrains/jewel/markdown/extensions/MarkdownBlockRendererExtension; | ||
} | ||
|
||
public final class org/jetbrains/jewel/markdown/extensions/tables/TableStyling { | ||
public static final field $stable I | ||
public static final field Companion Lorg/jetbrains/jewel/markdown/extensions/tables/TableStyling$Companion; | ||
public synthetic fun <init> (JJLkotlin/jvm/internal/DefaultConstructorMarker;)V | ||
public fun equals (Ljava/lang/Object;)Z | ||
public final fun getBorderColor-0d7_KjU ()J | ||
public final fun getHeadColor-0d7_KjU ()J | ||
public fun hashCode ()I | ||
public fun toString ()Ljava/lang/String; | ||
} | ||
|
||
public final class org/jetbrains/jewel/markdown/extensions/tables/TableStyling$Companion { | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
plugins { | ||
jewel | ||
`jewel-publish` | ||
`jewel-check-public-api` | ||
alias(libs.plugins.composeDesktop) | ||
} | ||
|
||
dependencies { | ||
implementation(projects.markdown.core) | ||
implementation(libs.commonmark.ext.gfm.tables) | ||
|
||
testImplementation(compose.desktop.uiTestJUnit4) | ||
} | ||
|
||
publicApiValidation { | ||
// TODO Oleg remove this once migrated to value classes | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @obask there is a TODO here for ya :) (and in at least another module, too!) |
||
excludedClassRegexes = setOf("org.jetbrains.jewel.markdown.extensions.github.alerts.*") | ||
} | ||
|
||
publishing.publications.named<MavenPublication>("main") { | ||
val ijpTarget = project.property("ijp.target") as String | ||
artifactId = "jewel-markdown-extension-${project.name}-$ijpTarget" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,97 @@ | ||
package org.jetbrains.jewel.markdown.extensions.tables | ||
|
||
import androidx.compose.foundation.background | ||
import androidx.compose.foundation.border | ||
import androidx.compose.foundation.layout.Column | ||
import androidx.compose.foundation.layout.Row | ||
import androidx.compose.foundation.layout.RowScope | ||
import androidx.compose.foundation.layout.fillMaxSize | ||
import androidx.compose.foundation.layout.fillMaxWidth | ||
import androidx.compose.foundation.layout.padding | ||
import androidx.compose.runtime.Composable | ||
import androidx.compose.ui.Modifier | ||
import androidx.compose.ui.unit.dp | ||
import org.commonmark.ext.gfm.tables.TableBlock | ||
import org.commonmark.ext.gfm.tables.TableBody | ||
import org.commonmark.ext.gfm.tables.TableHead | ||
import org.commonmark.node.Node | ||
import org.jetbrains.jewel.markdown.InlineMarkdown | ||
import org.jetbrains.jewel.markdown.MarkdownBlock.CustomBlock | ||
import org.jetbrains.jewel.markdown.extensions.MarkdownBlockRendererExtension | ||
import org.jetbrains.jewel.markdown.rendering.InlineMarkdownRenderer | ||
import org.jetbrains.jewel.markdown.rendering.MarkdownBlockRenderer | ||
import org.jetbrains.jewel.markdown.rendering.MarkdownStyling | ||
import org.jetbrains.jewel.markdown.toInlineNode | ||
import org.jetbrains.jewel.ui.component.Text | ||
|
||
public class GitHubTableBlockRenderer( | ||
private val styling: TableStyling, | ||
private val rootStyling: MarkdownStyling, | ||
) : MarkdownBlockRendererExtension { | ||
|
||
override fun canRender(block: CustomBlock): Boolean = | ||
block is CustomBlock.DefaultCustomBlock && block.nativeBlock is TableBlock | ||
|
||
@Composable | ||
private fun RowScope.TableCell( | ||
text: List<InlineMarkdown>, | ||
inlineRenderer: InlineMarkdownRenderer, | ||
) { | ||
Text( | ||
text = inlineRenderer.renderAsAnnotatedString(text, rootStyling.paragraph.inlinesStyling), | ||
Modifier | ||
.border(1.dp, styling.borderColor) | ||
.weight(1.0f) | ||
.padding(8.dp), | ||
) | ||
} | ||
|
||
@Composable | ||
override fun render( | ||
block: CustomBlock, | ||
blockRenderer: MarkdownBlockRenderer, | ||
inlineRenderer: InlineMarkdownRenderer, | ||
) { | ||
if (block !is CustomBlock.DefaultCustomBlock) return | ||
|
||
val head = block.nativeBlock.firstChild as TableHead | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gotta love CommonMark APIs 🙃 Can we expose head and body as properties on our table block model? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't want to invoke the processor unless it's needed for something. Right now the |
||
val body = block.nativeBlock.lastChild as TableBody | ||
|
||
Column(Modifier.fillMaxSize().padding(16.dp)) { | ||
head.forEachChild { row -> | ||
Row(Modifier.background(styling.headColor)) { | ||
row.forEachChild { cell -> | ||
TableCell(text = cell.children().map { it.toInlineNode() }, inlineRenderer) | ||
} | ||
} | ||
} | ||
body.forEachChild { row -> | ||
Row(Modifier.fillMaxWidth()) { | ||
row.forEachChild { cell -> | ||
TableCell(text = cell.children().map { it.toInlineNode() }, inlineRenderer) | ||
} | ||
} | ||
} | ||
} | ||
} | ||
|
||
@Composable | ||
private fun Node.children(): List<Node> = buildList { | ||
var child = firstChild | ||
|
||
while (child != null) { | ||
add(child) | ||
child = child.next | ||
} | ||
} | ||
} | ||
|
||
@Composable | ||
private fun Node.forEachChild(action: @Composable (Node) -> Unit) { | ||
var child = firstChild | ||
|
||
while (child != null) { | ||
action(child) | ||
child = child.next | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
package org.jetbrains.jewel.markdown.extensions.tables | ||
|
||
import org.commonmark.ext.gfm.tables.TablesExtension | ||
import org.commonmark.parser.Parser.ParserExtension | ||
import org.commonmark.renderer.text.TextContentRenderer.TextContentRendererExtension | ||
import org.jetbrains.jewel.markdown.extensions.MarkdownProcessorExtension | ||
|
||
public object GitHubTableProcessorExtension : MarkdownProcessorExtension { | ||
|
||
override val parserExtension: ParserExtension = TablesExtension.create() as ParserExtension | ||
override val textRendererExtension: TextContentRendererExtension = TablesExtension.create() as TextContentRendererExtension | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
package org.jetbrains.jewel.markdown.extensions.tables | ||
|
||
import org.jetbrains.jewel.markdown.extensions.MarkdownBlockRendererExtension | ||
import org.jetbrains.jewel.markdown.extensions.MarkdownRendererExtension | ||
import org.jetbrains.jewel.markdown.rendering.MarkdownStyling | ||
|
||
public class GitHubTableRendererExtension( | ||
alertStyling: TableStyling, | ||
rootStyling: MarkdownStyling, | ||
) : MarkdownRendererExtension { | ||
|
||
override val blockRenderer: MarkdownBlockRendererExtension = | ||
GitHubTableBlockRenderer(alertStyling, rootStyling) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
package org.jetbrains.jewel.markdown.extensions.tables | ||
|
||
import androidx.compose.ui.graphics.Color | ||
import org.jetbrains.jewel.foundation.GenerateDataFunctions | ||
|
||
@GenerateDataFunctions | ||
public class TableStyling( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see any facility to set things like cell padding, min row height, etc. Are those coming later? Or simply weren't considered? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My goal was to add an initial implementation of tables and change it later to follow more specific requirements. So the answer is "weren't considered". |
||
public val headColor: Color, | ||
Comment on lines
+7
to
+8
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this a background or content color? Please name accordingly There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My initial plan was to make it work as an alpha version. (I didn't think adding tables would be so easy) But yes, we should probably set the right styling in place. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would want to avoid having a half-implemented table extension merged in, unless we're sure the rest of the behaviour/styling is ready to go quickly (one day?) |
||
public val borderColor: Color, | ||
) { | ||
|
||
public companion object | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
package org.jetbrains.jewel.intui.markdown.styling.extension.github.alerts | ||
|
||
import androidx.compose.ui.graphics.Color | ||
import org.jetbrains.jewel.markdown.extensions.tables.TableStyling | ||
|
||
public fun TableStyling.Companion.dark( | ||
headColor: Color = Color.DarkGray, | ||
borderColor: Color = Color.Gray, | ||
Comment on lines
+7
to
+8
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are these defaults taken from the GitHub CSS, or arbitrary? If arbitrary, please get the actual values from the GitHub CSS (I recommend creating a table in a gist and inspecting in both light and dark themes) |
||
): TableStyling = TableStyling( | ||
headColor, | ||
borderColor, | ||
) | ||
|
||
public fun TableStyling.Companion.light( | ||
headColor: Color = Color.LightGray, | ||
borderColor: Color = Color.Gray, | ||
): TableStyling = TableStyling( | ||
headColor, | ||
borderColor, | ||
) |
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 aren't we just dropping blocks that can't get processed?
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.
Not sure I'm following what you mean here: are asking why we are keeping noop extensions? I'm planning to fix that later... my ideal API would just run
tryProcess
and only register extensions if they do something.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.
No, I am asking why we have an Elvis that returns a DefaultCustomBlock, which is something that really makes no sense to me. If it's a custom block, the concept of default is meaningless. We should not have it; instead, every extension should have its own custom block(s) — e.g.,
TableBlock(...): CustomBlock