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.

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

feat(gnoweb): Add column extension #3763

Open
wants to merge 27 commits into
base: master
Choose a base branch
from

Conversation

gfanton
Copy link
Member

@gfanton gfanton commented Feb 16, 2025

Addresses #3255

This PR implements column extension into gnoweb following this format:

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

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

Screenshot 2025-02-17 at 09 36 44

example with images (empty heading)

image

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 )

@gfanton gfanton requested review from moul and alexiscolin February 16, 2025 21:12
@gfanton gfanton self-assigned this Feb 16, 2025
@github-actions 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
@Gno2D2
Copy link
Collaborator

Gno2D2 commented Feb 16, 2025

🛠 PR Checks Summary

All Automated Checks passed. ✅

Manual Checks (for Reviewers):
  • IGNORE the bot requirements for this PR (force green CI check)
Read More

🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers.

✅ Automated Checks (for Contributors):

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

☑️ Contributor Actions:
  1. Fix any issues flagged by automated checks.
  2. 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:
  1. 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

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]>
Copy link

codecov bot commented Feb 16, 2025

Codecov Report

Attention: Patch coverage is 90.37433% with 18 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
gno.land/pkg/gnoweb/markdown/ext_column.go 90.21% 17 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@alexiscolin
Copy link
Member

Let's go ❤️

@aeddi
Copy link
Contributor

aeddi commented Feb 17, 2025

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.
Screenshot 2025-02-17 at 09 36 44

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
Copy link
Member

@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
Copy link
Contributor

aeddi commented Feb 17, 2025

@alexiscolin Ok got it 👍

@gfanton
Copy link
Member Author

gfanton commented Feb 20, 2025

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
Copy link
Member

alexiscolin commented Feb 20, 2025

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.

gfanton and others added 2 commits February 20, 2025 19:10
@aeddi aeddi self-assigned this Feb 20, 2025
@gfanton gfanton changed the title wip(gnoweb): Add column extension feat(gnoweb): Add column extension Feb 21, 2025
@gfanton gfanton marked this pull request as ready for review February 21, 2025 02:59
@gfanton
Copy link
Member Author

gfanton commented Feb 21, 2025

@leohhhn where should we document this feature ?

@alexiscolin
Copy link
Member

@leohhhn where should we document this feature ?

Related to this, yesterday, @jeronimoalbi asked for a place where all the Markdown extensions would be displayed. I mentioned r/demo/markdown_test/markdown.gno, but I think it would make more sense to have something in the documentation or elsewhere to gather them all. What do you think? @leohhhn

@leohhhn
Copy link
Contributor

leohhhn commented Feb 21, 2025

Let's start by creating a realm in the r/docs path, showcasing the functionality; I'd also love to see the old realms that use <div class="columns-1> refactored to use the new column system. I can help with this, just let me know what we converged on for the format

Some realms/packages that use the html columns are p/demo/blog, r/gnoland/events, r/leon/hof. Ideally, we would also be able to update md librarires to make adding columns even easier (ie p/moul/md).

We can add a showcase to r/demo/markdown_test but let's use r/docs from now to showcase these kinds of features. We can also add a short section on Markdown in effective gno.

@aeddi
Copy link
Contributor

aeddi commented Feb 21, 2025

Ok, so after discussing it with @alexiscolin and thinking about the edge case, I've got two points I would like to discuss before potentially implementing them myself if needed and if @gfanton agrees.

Nested tags

The first point concerns the case of an input containing <gno-column> nested tags.

Input
<gno-columns>
## title 1
content 1

<gno-columns>
## title 2
content 2

## title 3
content 3
</gno-columns>

## title 4
content 
</gno-columns>

Instead of producing an output that tries to "guess" which opening/closing tags should be ignored based on nesting depth, I'm suggesting we do something simpler, with less intelligence, and less likely to cause issues IMO.

Current output
<div class="gno-cols">
<!-- Column 1 -->
<div>
  <h2>title 1</h2>
  <p>content 1</p>
</div>
<!-- Column 2 -->
<div>
  <h2>title 2</h2>
  <p>content 2</p>
</div>
<!-- Column 3 -->
<div>
  <h2>title 3</h2>
  <p>content 3</p>
</div>
<!-- Column 4 -->
<div>
  <h2>title 4</h2>
  <p>content 4</p>
</div>
</div>

Option A

The first option would simply be to have a isTagOpened boolean that's true as soon as we parse an opening tag and remains true until we parse the corresponding closing tag. Then, apply the following logic (pseudo-code):

if isTagOpened && currentToken == openingTag {
	return // Just ignore this invalid opening tag.
}
if !isTagOpened && currentToken == closingTag {
  	return // Just ignore this invalid closing tag.
}

So the input above would then be processed as follows by the parser.

Input
<gno-columns>  <- Open
## title 1
content 1

<gno-columns>  <- Ignore this invalid opening tag
## title 2
content 2

## title 3
content 3
</gno-columns> <- Close

## title 4
content 
</gno-columns> <- Ignore this invalid closing tag

And should produce the following output.

Output
<div class="gno-cols">
<!-- Column 1 -->
<div>
  <h2>title 1</h2>
  <p>content 1</p>
</div>
<!-- Column 2 -->
<div>
  <h2>title 2</h2>
  <p>content 2</p>
</div>
<!-- Column 3 -->
<div>
  <h2>title 3</h2>
  <p>content 3</p>
</div>
</div>

<h2>title 4</h2>
<p>content 4</p>

Option B

I looked for a standard behavior to apply by trying to nest identical tags that shouldn't be nested in HTML. The problem is that most HTML tags nest within themselves just fine. I had the idea to test it with elements like ⁠<button> and it gave me this result.

So the behavior is as follows (tested on Firefox and Brave):

if isTagOpened && currentToken == openingTag {
	closePrevColumn() // Automatically close the previous tag if a new opening one is read
	openNewColumn()
}
if !isTagOpened && currentToken == closingTag {
  	return // Just ignore this invalid closing tag.
}

So the input above would then be processed as follows by the parser.

Input
<gno-columns>           <- Open 1
## title 1
content 1

<!-- </gno-columns> --> <- Automatically close 1 before opening 2
<gno-columns>           <- Open 2
## title 2
content 2

## title 3
content 3
</gno-columns>          <- Close 2

## title 4
content 
</gno-columns>          <- Ignore this invalid closing tag

And should produce the following output.

Output
<div class="gno-cols">
<!-- Column 1 -->
<div>
  <h2>title 1</h2>
  <p>content 1</p>
</div>
</div>

<div class="gno-cols">
<!-- Column 1 -->
<div>
  <h2>title 2</h2>
  <p>content 2</p>
</div>
<!-- Column 2 -->
<div>
  <h2>title 3</h2>
  <p>content 3</p>
</div>
</div>

<h2>title 4</h2>
<p>content 4</p>

Column not starting with a heading

For me, the ideal would be to have separators to make the division of column content explicit and easily readable by both humans and the parser. But sticking to the current system, consider an input with an opening tag not immediately followed by a heading.

Input
<gno-columns>
content 1
content 2

## Title 1
content 3

## Title 2
content 4
</gno-columns>

Currently, we produce an "invalid" column with the first lines, then we use the first heading we parse as a reference for defining subsequent columns.

Output
<div class="gno-cols">
<!-- Column 1 -->
<div>
  <p>content 1</p>
  <p>content 2</p>
</div>
<!-- Column 2 -->
<div>
  <h2>title 1</h2>
  <p>content 3</p>
</div>
<!-- Column 3 -->
<div>
  <h2>title 2</h2>
  <p>content 4</p>
</div>
</div>

Option A

We simply ignore the opening tag if it's not followed by a heading, and then we skip the closing tag since there wasn't a valid opening tag.

So the input above would then be processed as follows by the parser.

Input
<gno-columns>  <- Ignore this invalid opening tag (not followed by a heading)
content 1
content 2

## Title 1
content 3

## Title 2
content 4
</gno-columns> <- Ignore this invalid closing tag (no previous valid opening tag)

And should produce the following output.

Output
<p>content 1</p>
<p>content 2</p>

<h2>title 1</h2>
<p>content 3</p>

<h2>title 2</h2>
<p>content 4</p>

Option B

We put everything between <gno-columns> and </gno-columns> in the same column.

So the input above would then be processed as follows by the parser.

Input
<gno-columns>  <- Open
content 1
content 2

## Title 1
content 3

## Title 2
content 4
</gno-columns> <- Close

And should produce the following output.

Output
<div class="gno-cols">
<!-- Column 1 -->
<p>content 1</p>
<p>content 2</p>

<h2>title 1</h2>
<p>content 3</p>

<h2>title 2</h2>
<p>content 4</p>
</div>

Personally, in both cases I prefer option A, which basically means: "If the current tag is invalid, then just ignore it", which seems much clean and safe to me.

@gfanton
Copy link
Member Author

gfanton commented Feb 21, 2025

@aeddi thanks for the feedback, this is really pertinent, and I think you right, we should avoid putting too much inteligence in this and keep thing simple.

Nested tags

Optiona A: I think this is the correct behavior; removing depth simplifies the logic and still makes things pretty intuitive in terms of results.

Column not starting with a heading

You might be right with option A, but I'm not sure. I don't feel this is intuitive and might even be confusing. Ignoring the whole tag because of intermediary content might be too a bit too aggressive for me as the column are totally ignored . Also, you need to parse the next slibbing (not parse the next line), which may require some backtracking.

Options C

Wdyt about skipping content until finding a heading? and probably putting the content outside the cols.

Output
<p>content 1</p>
<p>content 2</p>

<div class="gno-cols">
<!-- Column 1 -->
<div>
  <h2>title 1</h2>
  <p>content 3</p>
</div>
<!-- Column 2 -->
<div>
  <h2>title 2</h2>
  <p>content 4</p>
</div>
</div>

In both case i don't like Option B

@aeddi
Copy link
Contributor

aeddi commented Feb 21, 2025

I agree that option C clearly seems better than B.

Option A is indeed more radical than B, but I think it might be the right approach. In blunt terms: "if it's invalid, we just consider it doesn't exist."

BTW, maybe we should display a warning message above the rendering of a Realm on gnoweb to indicate to the user that something is wrong? Like a summary/detail tag that indicates something is wrong with this rendering, followed by a list of warnings such as unexpected 'gno-column' closing tag at line 37.

But this is out of the scope of this PR anyway. Honestly, option C works for me as well, so I'll let you decide.

@leohhhn
Copy link
Contributor

leohhhn commented Feb 21, 2025

Guys, for a few examples of what people might do, check out the realms that I listed above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 ⛰️ gno.land Issues or PRs gno.land package related 🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

5 participants