feat(gnoweb): Add gno-columns extension #3763

gfanton · 2025-02-16T21:12:06Z

Addresses #3255

This PR implements column extension into gnoweb following this format:

-- input.md --
<gno-columns>
## Title 1
content 1
## Title 2
content 2
</gno-columns>

Notes

Check testdatas files for example of input/output
i've also added an example in /r/demo/mardown_test that need to be improved with usage, example and description (cc @leohhhn)

Preview

example output code

example with images (empty heading)

TODO

In order to support headings without creating columns, we probably want to use the first heading as a reference for column separator
More edge cases for tests
Add css to correctly render column in gnoweb (cc @alexiscolin )

Signed-off-by: gfanton <[email protected]>

Gno2D2 · 2025-02-16T21:13:09Z

🛠 PR Checks Summary

All Automated Checks passed. ✅

Manual Checks (for Reviewers):

IGNORE the bot requirements for this PR (force green CI check)

✅ Automated Checks (for Contributors):

🟢 Maintainers must be able to edit this pull request (more info)
🟢 Changes related to gnoweb must be reviewed by its codeowners

☑️ Contributor Actions:

Fix any issues flagged by automated checks.
Follow the Contributor Checklist to ensure your PR is ready for review.
- Add new tests, or document why they are unnecessary.
- Provide clear examples/screenshots, if necessary.
- Update documentation, if required.
- Ensure no breaking changes, or include BREAKING CHANGE notes.
- Link related issues/PRs, where applicable.

☑️ Reviewer Actions:

Complete manual checks for the PR, including the guidelines and additional checks if applicable.

📚 Resources:

Debug

Automated Checks

Maintainers must be able to edit this pull request (more info)

If

🟢 Condition met
└── 🟢 And
    ├── 🟢 The base branch matches this pattern: ^master$
    └── 🟢 The pull request was created from a fork (head branch repo: gfanton/gno)

Then

🟢 Requirement satisfied
└── 🟢 Maintainer can modify this pull request

Changes related to gnoweb must be reviewed by its codeowners

If

🟢 Condition met
└── 🟢 And
    ├── 🟢 The base branch matches this pattern: ^master$
    └── 🟢 A changed file matches this pattern: ^gno.land/pkg/gnoweb/ (filename: gno.land/pkg/gnoweb/components/layouts/article.html)

Then

🟢 Requirement satisfied
└── 🟢 Or
    ├── 🟢 This user reviewed pull request: alexiscolin
    └── 🟢 This user reviewed pull request: gfanton

Manual Checks

**IGNORE** the bot requirements for this PR (force green CI check)
If
🟢 Condition met
└── 🟢 On every pull request
Can be checked by

Any user with comment edit permission

Signed-off-by: gfanton <[email protected]>

codecov · 2025-02-16T21:26:09Z

Codecov Report

Attention: Patch coverage is 79.83539% with 49 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
gno.land/pkg/gnoweb/markdown/ext_column.go	85.11%	25 Missing ⚠️
gno.land/pkg/gnoweb/markdown/golden.go	66.66%	21 Missing and 3 partials ⚠️

📢 Thoughts on this report? Let us know!

alexiscolin · 2025-02-17T03:53:53Z

Let's go ❤️

Signed-off-by: gfanton <[email protected]>

aeddi · 2025-02-17T12:55:55Z

Cool, looks good :) so you finally decided to split the columns based on the heading or an image right instead of the separator right? (I mean <gno-col-1>, <gno-col-2>)

Honestly, it might be a bit less verbose without the separators, but I find it less intuitive to write/understand and harder to read. When I look at your example comparing the base code and the generated code, I find the generated code way more readable.

It also reduces the flexibility of what content can be added to the columns (something that not start with an image or a heading, or something not consistent between columns).

alexiscolin · 2025-02-17T14:36:57Z

@aeddi Yeah I agree regarding the readability but after some discussion in the dedicated issue, it seems people preferred the writing simplicity coupled with an opinionated gnoweb rendering based on headings (not images). To get the images, the hack is to create an empty heading. Please note that <gno-columns> has a shorthand :::.

aeddi · 2025-02-17T16:06:23Z

@alexiscolin Ok got it 👍

gfanton · 2025-02-20T03:52:53Z

There is one last edge case that I’m unsure how to handle:

<gno-columns>

content 1  
content 2  

## Title 1  

content 3  

## Title 2  

content 4  

</gno-columns>

In this scenario, there is intermediary content between the opening tag and the first heading.
While I would neither recommend nor document this, I want to ensure that any invalid format is handled in a predictable way.

Therefore, I suggest that we always create a first column, regardless of whether a heading is present.

alexiscolin · 2025-02-20T05:51:20Z

After some time to think about this, and considering that this edge case occurs because the first encountered title following the opening tag <gno-columns> is used as a separator marker, I agree that we should create a fallback column in this case. But we must ensure all the content before the first heading is embedded into a div.

Signed-off-by: gfanton <[email protected]>

alexiscolin · 2025-02-28T10:01:40Z

I have to check if the case "4 or more cols" can be rendered properly in one line with the current CSS.

Signed-off-by: gfanton <[email protected]>

gfanton · 2025-03-03T11:24:55Z

I've updated with option A in both scenarios.

For Column not starting with a heading : Honestly, I'm not sure about having option B, but it's not like option A is perfect either. This is mostly due to the chosen format, which is ambiguous by nature by reusing the current syntax as a separator, so any solution for intermediary content seems hacky anyway.
Also, don't forget that it is basically an error to not have a heading following the open tag. We should neither document nor support this syntax; we just want a predictable behavior. This also means we can update invalid behavior if it no longer meets our needs.

In addition, I've updated our approach to handling golden tests by using the txtar format. This provides us with a single source of truth for testing. I have also implemented pseudo error handling through an HTML comment.

@aeddi @alexiscolin double-check txtar to see if the output format suits you

aeddi

The implementation seems good to me and I think the code generation is more coherent now. Can you just fix the few comments I left you and the lint errors, and then I think we'll be good. 👍

aeddi · 2025-03-04T16:59:26Z

gno.land/pkg/gnoweb/markdown/ext_column.go

+		return false
+	}
+
+	fmt.Println(node.Kind())


aeddi · 2025-03-04T17:02:50Z

gno.land/pkg/gnoweb/markdown/ext_column.go

+	if len(line) == 0 {
+		return ColumnTagUndefined
+	}


The check seems unnecessary given the rest of the function.

This check has to be done at some point since we have to potentially check the first character of the line later in the function.

Oh yes, ok, just got the line 135.
WDYT about putting them together then so it's more clear? Like this:

if len(line) > 0 && line[0] == '#' { return ColumnTagSep }

(Or even using strings.HasPrefix)

gno.land/pkg/gnoweb/markdown/ext_column.go

gno.land/pkg/gnoweb/markdown/golden/ext_column/invalid_scopping_columns.golden.txtar

aeddi · 2025-03-04T18:46:45Z

gno.land/pkg/gnoweb/markdown/ext_column.go

+	IsClose, IsOpen bool
+	Error           error
+	Index           int
+	RefHeadingLevel int // serves as a level reference for separators


Why are these fields exported? Leftover or Is it for encoding (or any similar operation)? Can you unexport them or add a comment about this?

aeddi · 2025-03-04T18:51:36Z

gno.land/pkg/gnoweb/markdown/ext_column.go

+	if cctx.Error != nil {
+		// Don't bother with malformed block
+		return nil, parser.NoChildren
+	}


This check seems useless since getColumnContext return a new context if cctx.Error != nil.
For more clarity you could maybe just remove this getColumnContext function and move its content here?

aeddi · 2025-03-04T18:58:10Z

gno.land/pkg/gnoweb/markdown/ext_column.go

+	return ColumnTagUndefined
+}
+
+// Open opens a new column node based on the separator kind.


It might be me misunderstanding, but this comment seems unclear or even incorrect given the content of the function.

aeddi · 2025-03-04T19:09:31Z

gno.land/pkg/gnoweb/markdown/ext_column.go

+		prev, ok := cnode.PreviousSibling().(*ColumnNode)
+		if !ok || prev.Tag != ColumnTagOpen {
+			fmt.Fprintln(w, "</div>")
+		}


Can you comment this part? I'm not sure to understand its purpose.

it's to avoid having to close a non-existing sep, but re reading this I think it should easier to just check index.

aeddi · 2025-03-04T19:16:10Z

gno.land/pkg/gnoweb/markdown/ext_column.go

+
+// columnContext struct and its methods are used for handling column context.
+type columnContext struct {
+	IsClose, IsOpen bool


After reading the code related to these booleans, the fact that there are two doesn't seem necessary and is confusing.
Why not have a single boolean ⁠IsOpen? (or ⁠IsClosed if you want to init a new columnContext with its false value by default)?

Signed-off-by: gfanton <[email protected]>

examples/gno.land/r/demo/markdown_test/markdown.gno

…e content (#3909) once merged, we should take the habit of updating it when we extend gnoweb's rendering capabilities such as in #3763 --------- Signed-off-by: moul <[email protected]>

moul · 2025-03-11T17:21:23Z

i moved the markdown_test realm into r/docs/markdown #3909

can you update this PR?

moul · 2025-03-11T17:22:58Z

examples/gno.land/r/demo/markdown_test/markdown.gno

+content 2
+</gno-columns>
+
+You can also use ` + "`:::`" + ` as a shortcut for creating columns:


let's start with just <gno-columns>.

::: seems too 'precious' to be an alternative to something that's already concise and clear enough.
if ever we really want it, we can add it in another PR.

moul · 2025-03-11T17:23:50Z

gno.land/pkg/gnoweb/markdown/ext.go

+	"github.com/yuin/goldmark"
+)
+
+type gno struct{}


this code is unclear; can you add a comment?

moul · 2025-03-11T17:24:31Z

gno.land/pkg/gnoweb/markdown/ext_column.go

+
+	switch cnode.Tag {
+	case ColumnTagOpen:
+		fmt.Fprint(w, `<div class="gno-cols">`+"\n")


Suggested change

fmt.Fprint(w, `<div class="gno-cols">`+"\n")

fmt.Fprint(w, `<div class="gno-columns">`+"\n")

to stay consistent.

moul · 2025-03-11T17:26:20Z

gno.land/pkg/gnoweb/markdown/ext_column.go

ext_columns.go

consistency

moul · 2025-03-11T17:26:32Z

...land/pkg/gnoweb/markdown/golden/ext_column/invalid_beyond_maximum_level_heading.golden.txtar

ext_columns

consistency

moul · 2025-03-11T17:28:18Z

...land/pkg/gnoweb/markdown/golden/ext_column/invalid_beyond_maximum_level_heading.golden.txtar

can we avoid having those ".txtar" files that are really different from the other txtar files in the repo?

it looks more like a filetest.gno file in the spirit (one entrypoint, one output).

maybe ".mdtest"

moul · 2025-03-11T17:30:09Z

gno.land/pkg/gnoweb/markdown/ext_column.go

+	RefHeadingLevel int // serves as a level reference for separators
+}
+
+func isPreviousNodeTag(node ast.Node, tag ColumnTag) bool {


deadcode (and in general there are a lot of hard to read code) can you make a quick cleanup?

moul · 2025-03-11T17:31:09Z

gno.land/pkg/gnoweb/markdown/ext_column.go

+	return ok && cnode.Tag == tag
+}
+
+// parseLineTag checks if the line starts with any of the tag.


incorrect, the code checks if the line is a tag, not if it starts with.

it actually doesn't support having a tag and something else on the right, which will be a need for upcoming extensions.

moul · 2025-03-11T17:31:32Z

gno.land/pkg/gnoweb/markdown/ext_column.go

+
+func (*columnParser) Close(_ ast.Node, reader text.Reader, _ parser.Context) {}
+
+func (*columnParser) CanInterruptParagraph() bool {


deadcode? if not add a comment

Those methods are for block parsing interface; I will add an assert reference.

moul · 2025-03-11T17:31:41Z

gno.land/pkg/gnoweb/markdown/ext_column.go

+	return true
+}
+
+func (*columnParser) CanAcceptIndentedLine() bool {


moul · 2025-03-11T17:32:54Z

gno.land/pkg/gnoweb/markdown/ext_test.go

+const testdataDir = "golden"
+
+var (
+	update = flag.Bool("update-golden-files", false, "update golden files")


Suggested change

update = flag.Bool("update-golden-files", false, "update golden files")

update = flag.Bool("update-golden-tests", false, "update golden files")

for consistency

feat(gnoweb): Add column extension

58f47b5

Signed-off-by: gfanton <[email protected]>

gfanton requested review from moul and alexiscolin February 16, 2025 21:12

gfanton self-assigned this Feb 16, 2025

github-actions bot added 🧾 package/realm Tag used for new Realms or Packages. 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Feb 16, 2025

chore: rename

3490ccb

Signed-off-by: gfanton <[email protected]>

gfanton assigned alexiscolin Feb 16, 2025

alexiscolin and others added 8 commits February 17, 2025 14:42

fix: update columns tag

988e691

style: add cols css

9241afe

feat: first title level as separator

84e5e9d

fix: use first heading a ref for separator

2b62c9c

Signed-off-by: gfanton <[email protected]>

fix: golden + cols

6549bf7

Signed-off-by: gfanton <[email protected]>

fix: lint & typo

cb13eee

Signed-off-by: gfanton <[email protected]>

Merge branch 'master' into feat/gnoweb/markdown-column

5a22e46

chore: lint

5810986

Signed-off-by: gfanton <[email protected]>

fix: remove col class

e187f25

zivkovicmilos requested review from zivkovicmilos and aeddi February 17, 2025 16:01

alexiscolin and others added 3 commits February 20, 2025 15:14

chore: add tests

8974890

chore: update markdown doc

a8d8f84

Signed-off-by: gfanton <[email protected]>

feat: add column0

6b5f404

Signed-off-by: gfanton <[email protected]>

gfanton added 3 commits March 3, 2025 22:03

feat(md): use txtar as golden files

5315edc

Signed-off-by: gfanton <[email protected]>

feat: make columns more strict

af1c6c0

Signed-off-by: gfanton <[email protected]>

fix: update golden files

6459b09

Signed-off-by: gfanton <[email protected]>

style: max 4 cols in a row

fec4c3a

aeddi requested changes Mar 4, 2025

View reviewed changes

gfanton added 6 commits March 6, 2025 11:20

chore: remove leftover

3044217

Signed-off-by: gfanton <[email protected]>

chore: cleanup

c1ba81c

Signed-off-by: gfanton <[email protected]>

chore: rename golden file

355f5d8

Signed-off-by: gfanton <[email protected]>

chore: cleanup comments

597b128

Signed-off-by: gfanton <[email protected]>

chore: improve closing tag

10634bc

Signed-off-by: gfanton <[email protected]>

chore: cleanup useless code

7e859dc

Signed-off-by: gfanton <[email protected]>

leohhhn reviewed Mar 6, 2025

View reviewed changes

examples/gno.land/r/demo/markdown_test/markdown.gno Show resolved Hide resolved

alexiscolin mentioned this pull request Mar 7, 2025

refactor(WIP): gnoland home realm #3888

Draft

5 tasks

moul mentioned this pull request Mar 11, 2025

chore(examples): move r/demo/markdown_test to r/docs/markdown + update content #3909

Merged

moul changed the title ~~feat(gnoweb): Add column extension~~ feat(gnoweb): Add gno-columns extension Mar 11, 2025

moul reviewed Mar 11, 2025

View reviewed changes

gno.land/pkg/gnoweb/markdown/ext.go

"github.com/yuin/goldmark"

)

type gno struct{}

Copy link

Member

moul Mar 11, 2025

Choose a reason for hiding this comment

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

this code is unclear; can you add a comment?

moul reviewed Mar 11, 2025

View reviewed changes

gno.land/pkg/gnoweb/markdown/ext_column.go

Copy link

Member

moul Mar 11, 2025

Choose a reason for hiding this comment

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

ext_columns.go

consistency

moul reviewed Mar 11, 2025

View reviewed changes

...land/pkg/gnoweb/markdown/golden/ext_column/invalid_beyond_maximum_level_heading.golden.txtar

Copy link

Member

moul Mar 11, 2025

Choose a reason for hiding this comment

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

ext_columns

consistency

moul reviewed Mar 11, 2025

View reviewed changes

	fmt.Fprint(w, `<div class="gno-cols">`+"\n")
	fmt.Fprint(w, `<div class="gno-columns">`+"\n")


		func (*columnParser) Close(_ ast.Node, reader text.Reader, _ parser.Context) {}

		func (*columnParser) CanInterruptParagraph() bool {

	update = flag.Bool("update-golden-files", false, "update golden files")
	update = flag.Bool("update-golden-tests", false, "update golden files")

feat(gnoweb): Add gno-columns extension #3763

Are you sure you want to change the base?

feat(gnoweb): Add gno-columns extension #3763

Conversation

gfanton commented Feb 16, 2025 • edited by moul Loading

Notes

Preview

example output code

example with images (empty heading)

TODO

Gno2D2 commented Feb 16, 2025 • edited Loading

🛠 PR Checks Summary

Manual Checks (for Reviewers):

✅ Automated Checks (for Contributors):

☑️ Contributor Actions:

☑️ Reviewer Actions:

📚 Resources:

If

Then

If

Then

If

Can be checked by

codecov bot commented Feb 16, 2025 • edited Loading

Codecov Report

alexiscolin commented Feb 17, 2025

aeddi commented Feb 17, 2025

alexiscolin commented Feb 17, 2025

aeddi commented Feb 17, 2025

gfanton commented Feb 20, 2025

alexiscolin commented Feb 20, 2025 • edited Loading

alexiscolin commented Feb 28, 2025 • edited Loading

gfanton commented Mar 3, 2025

aeddi left a 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

moul commented Mar 11, 2025

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

moul Mar 11, 2025 • edited Loading

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

gfanton commented Feb 16, 2025 •

edited by moul

Loading

Gno2D2 commented Feb 16, 2025 •

edited

Loading

codecov bot commented Feb 16, 2025 •

edited

Loading

alexiscolin commented Feb 20, 2025 •

edited

Loading

alexiscolin commented Feb 28, 2025 •

edited

Loading

moul Mar 11, 2025 •

edited

Loading