Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[JEWEL-735] Implement Markdown tables support (GFM extension) #2913

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[JEWEL-735] Implement Markdown tables support (GFM extension) #2913

wants to merge 1 commit into from

Conversation

hamen
Copy link
Collaborator

@hamen hamen commented Jan 16, 2025

@hamen hamen requested a review from AlexVanGogen January 16, 2025 17:27
This was referenced Jan 17, 2025
Open
Open
@jakub-senohrabek
Copy link
Collaborator

Is this still not passing the checks, or is the result old?

@rock3r
Copy link
Collaborator

rock3r commented Jan 20, 2025

@jakub-senohrabek we're waiting for Sasha to take a look at the scroll sync tests

@rock3r rock3r self-assigned this Jan 20, 2025
AlexVanGogen
AlexVanGogen reviewed Jan 20, 2025
View reviewed changes
Copy link
Contributor

@AlexVanGogen AlexVanGogen left a comment

Choose a reason for hiding this comment

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

Haven't checked the code itself yet

.idea/runConfigurations/_template__of_TestNG.xml Outdated Show resolved Hide resolved
.idea/vcs.xml Outdated Show resolved Hide resolved
platform/jewel/.idea/inspectionProfiles/Project_Default.xml Outdated Show resolved Hide resolved
platform/jewel/art/docs/custom-chrome.png Outdated Show resolved Hide resolved
platform/jewel/art/docs/markdown-renderer.png Outdated Show resolved Hide resolved
...wel/markdown/ide-laf-bridge-styling/intellij.platform.jewel.markdown.ideLafBridgeStyling.iml Outdated Show resolved Hide resolved
...rkdown/int-ui-standalone-styling/intellij.platform.jewel.markdown.intUiStandaloneStyling.iml Outdated Show resolved Hide resolved
platform/jewel/samples/standalone/intellij.platform.jewel.samples.standalone.iml Outdated Show resolved Hide resolved
.idea/modules.xml Outdated Show resolved Hide resolved
.../core/src/main/kotlin/org/jetbrains/jewel/markdown/rendering/DefaultMarkdownBlockRenderer.kt Show resolved Hide resolved
@rock3r rock3r force-pushed the im-jewel-735 branch 2 times, most recently from 93b2beb to 4e80a98 Compare January 20, 2025 16:33
@rock3r
Copy link
Collaborator

rock3r commented Jan 20, 2025

@AlexVanGogen reverted everything that needed reverting. Had to do a force push because Ivan had done git merge, and that commit was what fucked up the stuff you picked up on.

(FYI @hamen please do not do merges, use rebase.)

@rock3r
Copy link
Collaborator

rock3r commented Jan 20, 2025

PS: I can't mark your comments as resolved, but the ones that needed me to do something (i.e., revert changes in the default renderer, or renaming the iml module) are done.

@rock3r rock3r mentioned this pull request Jan 20, 2025
Closed
@rock3r
Copy link
Collaborator

rock3r commented Jan 21, 2025

@AlexVanGogen fixed all the new comments :)

@AlexVanGogen
Copy link
Contributor

@rock3r It almost compiles now 🎉 , there's one unupdated reference to not existing anymore gfm-tables.iml but that's trivial to fix by ourselves, especially given it has to be repeated anyway on the ultimate repo.

In the meantime, I've tried to paste my little testing table to a standalone sample and got this:
Screenshot 2025-01-21 at 16 12 40

Is there an option to keep rows wide enough to avoid soft-wrapping? To get something like what GitHub renders by default:

Tables Are Cool
col 3 is right-aligned $1600
col 2 is centered $12
zebra stripes are neat $1

@rock3r
Copy link
Collaborator

rock3r commented Jan 21, 2025

@AlexVanGogen I never noticed that behaviour. It's possible there may be some bug in the table layout, it's brand new :) But I would not stop this PR over that

@rock3r rock3r force-pushed the im-jewel-735 branch 2 times, most recently from 341c2e5 to 6b9a42e Compare January 21, 2025 16:28
AlexVanGogen
AlexVanGogen requested changes Jan 21, 2025
View reviewed changes
Copy link
Contributor

@AlexVanGogen AlexVanGogen left a comment

Choose a reason for hiding this comment

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

LGTM, just left a few minor comments about the code.

On a sidenote, it's been feast for my eyes to finally see rendered tables in the previews, and not a mishmash of hyphens and pipes 😀

...m/jewel/foundation/src/main/kotlin/org/jetbrains/jewel/foundation/layout/BasicTableLayout.kt Outdated Show resolved Hide resolved
...m/jewel/foundation/src/main/kotlin/org/jetbrains/jewel/foundation/layout/BasicTableLayout.kt
// the (possibly scaled) intrinsic column widths we just computed
val intrinsicRowHeights = IntArray(rowCount)
var tableHeight = 0
measurablesByRow.mapIndexed { rowIndex, rowMeasurables ->
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
measurablesByRow.mapIndexed { rowIndex, rowMeasurables ->
measurablesByRow.forEachIndexed { rowIndex, rowMeasurables ->

Copy link
Collaborator

Choose a reason for hiding this comment

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

Was a bit larger change than this but 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

You probably meant the loop above, with withIndex :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big deal tho

...ewel/markdown/core/src/main/kotlin/org/jetbrains/jewel/markdown/processing/ProcessingUtil.kt Show resolved Hide resolved
...ewel/markdown/core/src/main/kotlin/org/jetbrains/jewel/markdown/processing/ProcessingUtil.kt Show resolved Hide resolved
...arkdown/core/src/main/kotlin/org/jetbrains/jewel/markdown/rendering/MarkdownBlockRenderer.kt Show resolved Hide resolved
...ain/kotlin/org/jetbrains/jewel/markdown/extensions/github/tables/GitHubTableBlockRenderer.kt Outdated Show resolved Hide resolved
...ain/kotlin/org/jetbrains/jewel/markdown/extensions/github/tables/GitHubTableBlockRenderer.kt Outdated Show resolved Hide resolved
...ain/kotlin/org/jetbrains/jewel/markdown/extensions/github/tables/GitHubTableBlockRenderer.kt Outdated Show resolved Hide resolved
@rock3r rock3r mentioned this pull request Jan 21, 2025
Open
@AlexVanGogen AlexVanGogen self-requested a review January 22, 2025 13:36
@AlexVanGogen
Copy link
Contributor

One tiny thing left, but I think that's ok to merge

@rock3r rock3r mentioned this pull request Jan 22, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexVanGogen AlexVanGogen AlexVanGogen approved these changes

@rock3r rock3r rock3r left review comments

Assignees

@rock3r rock3r

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hamen @jakub-senohrabek @rock3r @AlexVanGogen