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

Fix markdown preview $$ support #31514

Merged
merged 3 commits into from
Jun 29, 2024

Conversation

charles7668
Copy link
Contributor

close #31481

currently $$A + B$$ test will ignore text after $$ block

test text
圖片

before fix
圖片

after fix
圖片

github display
圖片

@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 28, 2024
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 28, 2024
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Jun 28, 2024
@wxiaoguang
Copy link
Contributor

I guess we shouldn't introduce too much copy&pasted code ........

@charles7668 charles7668 marked this pull request as draft June 28, 2024 01:59
@charles7668 charles7668 force-pushed the fix/support-dual-dollar-math-block branch from b42d294 to f347858 Compare June 28, 2024 03:03
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 28, 2024
@charles7668 charles7668 marked this pull request as ready for review June 28, 2024 03:37
@charles7668
Copy link
Contributor Author

charles7668 commented Jun 28, 2024

I guess we shouldn't introduce too much copy&pasted code ........

Removed some duplicate code.

if _, ok := n.(*InlineBlock); ok {
_, _ = w.WriteString(`<pre class="code-block is-loading"><code class="chroma language-math display">`)
} else {
_, _ = w.WriteString(`<code class="language-math is-loading">`)
Copy link
Contributor

Choose a reason for hiding this comment

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

It could be <code class="language-math is-loading {{some-extra-class-for-inline-or-not}}">

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could be <code class="language-math is-loading {{some-extra-class-for-inline-or-not}}">

is-loading is not a common code attribute in this case, and the class order can't be changed. If we need to change the order, we might need to update the rule in sanitizer_default.go.

Copy link
Contributor

Choose a reason for hiding this comment

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

My intuition is that single <code> is good enough and more maintainable/consistent, while I won't block the two tags.

Copy link
Member

Choose a reason for hiding this comment

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

I think the only reason for two tags is semantical correctness.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used two tags to keep the same style as the block render for the $$ block, but one code tag also works.

I have updated it to the one code tag version.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jun 28, 2024
@charles7668 charles7668 force-pushed the fix/support-dual-dollar-math-block branch from a288b49 to 650fbd3 Compare June 29, 2024 17:53
@charles7668 charles7668 force-pushed the fix/support-dual-dollar-math-block branch from 650fbd3 to 1a85932 Compare June 29, 2024 17:59
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 29, 2024
@lafriks lafriks enabled auto-merge (squash) June 29, 2024 22:56
@lafriks lafriks merged commit f003305 into go-gitea:main Jun 29, 2024
26 checks passed
@GiteaBot GiteaBot added this to the 1.23.0 milestone Jun 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/go Pull requests that update Go code size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

$$ Math Markup inside tables is not supported
5 participants