Skip to content

Add InlineCode option for inline code blocks #618

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
Merged
merged 1 commit into from
Jun 14, 2022
Merged

Add InlineCode option for inline code blocks #618

merged 1 commit into from
Jun 14, 2022

Conversation

CIAvash
Copy link
Contributor

@CIAvash CIAvash commented Mar 10, 2022 •
edited
Loading

Please test and review if this behavior is desired and correct.

Specially PreventSurroundingPre, is everyone OK with this behavior?

@CIAvash
Copy link
Contributor Author

CIAvash commented Mar 10, 2022

Forgot to ping @bep and @kaushalmodi

@kaushalmodi
Copy link
Contributor

@CIAvash This looks awesome!

I don't code in Go, but looking at the tests in this PR, it looks perfect!

@bep can comment on how the new InlineCode option can be interfaced with Hugo. I think the change in this PR is awesome and will remove the need to have hacks like this on the Hugo side:

https://github.com/gohugoio/hugo/blob/c235fcf042dfbdf79c21acc1fd05f140d17e558e/markup/highlight/highlight.go#L181-L184

@kaushalmodi kaushalmodi mentioned this pull request Mar 10, 2022
Closed
@stefanv stefanv mentioned this pull request Mar 10, 2022
Closed
@CIAvash CIAvash mentioned this pull request May 16, 2022
Merged
@CIAvash
Copy link
Contributor Author

CIAvash commented Jun 14, 2022

@alecthomas Have you made any decision on this? If you don't want to add InlineCode option now that there is WithCustomCSS. I think changes for PreventSurroundingPre is still needed.

Just tell me what to do when you've decided.

@alecthomas
Copy link
Owner

Oh sorry, I spaced on this. LGTM!

Fix the merge conflicts and we'll get it in.

@CIAvash
Add InlineCode option for inline code blocks
a598673 
- Add an `InlineCode` option for inline code blocks
- When `PreventSurroundingPre` option is enabled, do not wrap the code by `Line` and `CodeLine`
- Write and update related tests
@kaushalmodi
Copy link
Contributor

@CIAvash

I think changes for PreventSurroundingPre is still needed.

+1

Thanks for following up on this.

@alecthomas alecthomas merged commit d18e8a4 into alecthomas:master Jun 14, 2022
@alecthomas
Copy link
Owner

@bep not sure if you want to pull this in too, it's tagged as v2.2.0

@CIAvash CIAvash deleted the inline_code_fix branch June 14, 2022 11:39
@jolheiser jolheiser mentioned this pull request Jul 7, 2022
Merged
@silverwind silverwind mentioned this pull request Sep 25, 2022
Merged
@silverwind
Copy link
Contributor

The change to PreventSurroundingPre seems odd. Why should an option that is meant to only affect the whole-code wrapping also remove per-line wrapping? I think a option like a WithLineWrapper would be nice to have to control these two wrapping types individually.

lunny pushed a commit to go-gitea/gitea that referenced this pull request Sep 26, 2022
@silverwind @wxiaoguang
Upgrade chroma to v2.3.0 (#21259)
ec0a06e 
The behaviour of `PreventSurroundingPre` has changed in
alecthomas/chroma#618 so that apparently it now
causes line wrapper tags to be no longer emitted, but we need some form
of indication to split the HTML into lines, so I did what
yuin/goldmark-highlighting#33 did and added the
`nopWrapper`.

Maybe there are more elegant solutions but for some reason, just
splitting the HTML string on `\n` did not work.

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
@CIAvash
Copy link
Contributor Author

CIAvash commented Sep 26, 2022

The change to PreventSurroundingPre seems odd. Why should an option that is meant to only affect the whole-code wrapping also remove per-line wrapping? I think a option like a WithLineWrapper would be nice to have to control these two wrapping types individually.

I don't remember the reason now, but that's probably why I asked:

Specially PreventSurroundingPre, is everyone OK with this behavior?

And nobody gave any feedback.

Unrelated to this, I'm a bit curious why people don't want the pre. That may help understand the situation.

What do you think @alecthomas?

@silverwind
Copy link
Contributor

silverwind commented Sep 26, 2022 •
edited
Loading

We render our own <pre><code> with additional logic for class names and such. In go-gitea/gitea@ec0a06e, I discovered chroma's SplitTokensIntoLines which actually solves the issue for us, so we don't really need additional API.

@CIAvash
Copy link
Contributor Author

CIAvash commented Sep 26, 2022

For that WithPreWrapper can be used, that's what goldmark-highlighting used to remove pre and code tag.

The question about the correct behavior of PreventSurroundingPre still remains. Maybe the f.preventSurroundingPre check should be removed as you said the option is about pre, not lines.

@silverwind
Copy link
Contributor

silverwind commented Sep 26, 2022 •
edited
Loading

Yeah, I agree, preventSurroundingPre should ideally not have any impact on the per-line wrapping, it should just affect the overall code-block wrapping only. After this double-action is removed, there still needs to be a way to prevent per-line wrapping itself (maybe there already is, I did not check in detail).

@silverwind
Copy link
Contributor

So I think the best course of action would be to split the behaviour into two options:

PreventSurroundingPre prevents <pre><code> block wrapping
PreventLineWrapping prevents <span class="line"><span class="cl"> line wrapping

For consistency, there should also probably be a per-line wrapper options for people to supply their custom wrapper, e.g.

WithPreWrapper handles pre wrapping
WithLineWrapper handles line wrapping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Default line CSS display: flex; creating a problem if we want to inline a code snippet
4 participants
@CIAvash @kaushalmodi @alecthomas @silverwind