Add tables extension #339

obask · 2024-03-31T07:07:40Z

*please ignore commits before Wire up GFM Tables extension

obask · 2024-03-31T07:12:26Z

Also, maybe we could make most of the nodes data classes, but it will require merging some underlying CLs to make inline nodes value classes.

rock3r · 2024-03-31T17:01:39Z

@obask you'll need to sort out conflicts in this PR and in others for me to ship them. Please make sure you stack PRs that have dependencies.

obask · 2024-03-31T21:20:57Z

@obask you'll need to sort out conflicts in this PR and in others for me to ship them. Please make sure you stack PRs that have dependencies.

As I mentioned before, I don't have rights to push to existing branches making this impossible:
$ git push -u origin migrate-components

For example, this check is a requirement for feature branches https://github.com/JetBrains/jewel/blob/main/.github/workflows/codeql.yml#L7, but I don't think running it in a fork will give me an ability to push to origin aka JetBrains/jewel.
The only working solution for an existing setup would be to create a new stacked PR every time I "address comments".

rock3r · 2024-03-31T21:27:31Z

You have full write access to this repository, you shouldn't need to have a fork. I have weakened the branch protection rules even more than they already were. You should be able to push and force push to any branch directly with the exception of main.

obask · 2024-04-01T01:51:31Z

Let's sync on that over video when you have time. I'm getting the same error:

oleg@Olegs-MacBook-Air jewel % git commit -m 'Test branch protection rules'
[test-branch-protection-rules baced50] Test branch protection rules
 1 file changed, 1 insertion(+)
oleg@Olegs-MacBook-Air jewel % git push
Enumerating objects: 23, done.
Counting objects: 100% (23/23), done.
Delta compression using up to 8 threads
Compressing objects: 100% (7/7), done.
Writing objects: 100% (12/12), 785 bytes | 785.00 KiB/s, done.
Total 12 (delta 5), reused 0 (delta 0), pack-reused 0
remote: Resolving deltas: 100% (5/5), completed with 5 local objects.
remote: error: GH006: Protected branch update failed for refs/heads/test-branch-protection-rules.
remote: error: 2 of 2 required status checks are expected.
To github.com:JetBrains/jewel.git
 ! [remote rejected] test-branch-protection-rules -> test-branch-protection-rules (protected branch hook declined)
error: failed to push some refs to 'github.com:JetBrains/jewel.git'

rock3r · 2024-04-02T15:50:16Z

markdown/core/src/main/kotlin/org/jetbrains/jewel/markdown/processing/MarkdownProcessor.kt

@@ -189,8 +190,9 @@ public class MarkdownProcessor(
 is ThematicBreak -> MarkdownBlock.ThematicBreak
 is HtmlBlock -> toMarkdownHtmlBlockOrNull()
 is CustomBlock -> {
- extensions.find { it.processorExtension?.canProcess(this) == true }
+ extensions.find { it.processorExtension.canProcess(this) }


Why aren't we just dropping blocks that can't get processed?

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.

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

rock3r · 2024-04-02T15:51:10Z

markdown/extension/gfm-tables/build.gradle.kts

+}
+
+publicApiValidation {
+ // TODO Oleg remove this once migrated to value classes


Hi @obask there is a TODO here for ya :) (and in at least another module, too!)

rock3r · 2024-04-02T15:54:59Z

...s/src/main/kotlin/org/jetbrains/jewel/markdown/extensions/tables/GitHubTableBlockRenderer.kt

+ ) {
+ if (block !is CustomBlock.DefaultCustomBlock) return
+
+ val head = block.nativeBlock.firstChild as TableHead


Gotta love CommonMark APIs 🙃

Can we expose head and body as properties on our table block model?

I don't want to invoke the processor unless it's needed for something. Right now the CustomBlock is an interface and we can always convert it to data classes later if we need to.

rock3r · 2024-04-02T15:56:57Z

...on/gfm-tables/src/main/kotlin/org/jetbrains/jewel/markdown/extensions/tables/TableStyling.kt

+public class TableStyling(
+ public val headColor: Color,


Is this a background or content color? Please name accordingly

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.

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?)

rock3r · 2024-04-02T15:58:07Z

.../jetbrains/jewel/intui/markdown/styling/extension/github/alerts/GitHubTableDefaultStyling.kt

+ headColor: Color = Color.DarkGray,
+ borderColor: Color = Color.Gray,


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)

rock3r · 2024-04-02T15:59:08Z

...on/gfm-tables/src/main/kotlin/org/jetbrains/jewel/markdown/extensions/tables/TableStyling.kt

+import org.jetbrains.jewel.foundation.GenerateDataFunctions
+
+@GenerateDataFunctions
+public class TableStyling(


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?

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".

rock3r · 2024-04-02T18:27:51Z

Oh btw make sure all extensions are registered in the mergeSarif task — I suspect we may have forgotten that for previous PRs, too — not just this one. See

jewel/build.gradle.kts

Lines 8 to 21 in 5d33bd5

 dependencies { 

 sarif(projects.decoratedWindow) 

 sarif(projects.foundation) 

 sarif(projects.ideLafBridge) 

 sarif(projects.intUi.intUiDecoratedWindow) 

 sarif(projects.intUi.intUiStandalone) 

 sarif(projects.markdown.core) 

 sarif(projects.markdown.extension.gfmAlerts) 

 sarif(projects.markdown.ideLafBridgeStyling) 

 sarif(projects.markdown.intUiStandaloneStyling) 

 sarif(projects.samples.idePlugin) 

 sarif(projects.samples.standalone) 

 sarif(projects.ui) 

 }

obask self-assigned this Mar 31, 2024

obask requested a review from rock3r March 31, 2024 07:07

obask force-pushed the add-tables-extension branch from 4e0a262 to 49ce567 Compare March 31, 2024 22:04

obask force-pushed the add-tables-extension branch from 49ce567 to 13a02f7 Compare April 2, 2024 09:10

Wire up GFM Tables extension

cdf90a7

obask force-pushed the add-tables-extension branch from 13a02f7 to cdf90a7 Compare April 2, 2024 09:12

obask enabled auto-merge (squash) April 2, 2024 09:13

rock3r requested changes Apr 2, 2024

View reviewed changes

rock3r added the markdown This issue impacts the Markdown rendering subsystem label Jul 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tables extension #339

Add tables extension #339

obask commented Mar 31, 2024

obask commented Mar 31, 2024

rock3r commented Mar 31, 2024

obask commented Mar 31, 2024

rock3r commented Mar 31, 2024

obask commented Apr 1, 2024

rock3r Apr 2, 2024

obask Apr 2, 2024

rock3r Apr 2, 2024

rock3r Apr 2, 2024

rock3r Apr 2, 2024

obask Apr 2, 2024

rock3r Apr 2, 2024

obask Apr 2, 2024

rock3r Apr 2, 2024

rock3r Apr 2, 2024

rock3r Apr 2, 2024

obask Apr 2, 2024

rock3r commented Apr 2, 2024

		headColor: Color = Color.DarkGray,
		borderColor: Color = Color.Gray,

Add tables extension #339

Are you sure you want to change the base?

Add tables extension #339

Conversation

obask commented Mar 31, 2024

obask commented Mar 31, 2024

rock3r commented Mar 31, 2024

obask commented Mar 31, 2024

rock3r commented Mar 31, 2024

obask commented Apr 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rock3r commented Apr 2, 2024