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 image with link #4675

Merged
merged 6 commits into from Oct 30, 2018

Conversation

LER0ever
Copy link
Contributor

@LER0ever LER0ever commented Aug 12, 2018

This PR fixes the image with link rendering. And therefore resolves #4569 and resolves #5207

Original Behavior

Images with ![alt](imgurl) will be put inside a link to itself, unless it's an svg-file.

The problem is that it hardcoded the svg filetype to be directly rendered in <img> tag, and other file types are rendered like <a><img></a> which causes #4569 and #5207 .

New Behavior in this PR

Images with ![alt](imgurl) will be put inside a link to itself, unless it's already inside a link, which is [![alt](imgurl)](url).
So we don't need to infer from filetype whether the image is a CI image or not.
Also, this matches the Github-Flavored Markdown's behavior on images with links.

Copy link
Member

@techknowlogick techknowlogick left a comment

Choose a reason for hiding this comment

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

Can you add the Gitea copyright at the top of the file?

@bkcsoft bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 12, 2018
@LER0ever
Copy link
Contributor Author

Can you add the Gitea copyright at the top of the file?

@techknowlogick , what do you mean? This is not a new file, and the copyright is already there markdown.go

@techknowlogick
Copy link
Member

// Copyright 2018 The Gitea Authors. All rights reserved.

Needs to be added under the Gogs copyright

@LER0ever
Copy link
Contributor Author

I see, fixed in fdd8ceb .

@codecov-io
Copy link

codecov-io commented Aug 12, 2018

Codecov Report

Merging #4675 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4675      +/-   ##
==========================================
+ Coverage   37.47%   37.49%   +0.02%     
==========================================
  Files         310      310              
  Lines       45921    45922       +1     
==========================================
+ Hits        17207    17217      +10     
+ Misses      26239    26231       -8     
+ Partials     2475     2474       -1
Impacted Files Coverage Δ
modules/markup/markdown/markdown.go 73.14% <100%> (+6.79%) ⬆️
models/repo_list.go 64.55% <0%> (+1.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1037065...a3ba588. Read the comment docs.

@techknowlogick
Copy link
Member

@LER0ever Thanks 😄

@lafriks
Copy link
Member

lafriks commented Aug 12, 2018

Tests would be needed for this

@lafriks lafriks added the type/enhancement An improvement of existing functionality label Aug 12, 2018
@lafriks lafriks added this to the 1.6.0 milestone Aug 12, 2018
@LER0ever
Copy link
Contributor Author

@lafriks Tests added for images with links.

Meanwhile, although this implementation is definitely gonna work, I still find this character locating approach a little bit dirty.
So if anyone has a better one that also processes images with links the same way GitHub does, feel free to replace this one.

@techknowlogick techknowlogick modified the milestones: 1.6.0, 1.7.0 Sep 3, 2018
@LER0ever LER0ever mentioned this pull request Oct 30, 2018
7 tasks
@bkcsoft bkcsoft 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 Oct 30, 2018
@bkcsoft bkcsoft 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 Oct 30, 2018
@techknowlogick techknowlogick merged commit b686bd0 into go-gitea:master Oct 30, 2018
@lunny
Copy link
Member

lunny commented Nov 8, 2018

@LER0ever @lafriks This should be backport to v1.6

lafriks pushed a commit to lafriks-fork/gitea that referenced this pull request Nov 8, 2018
* Fix markdown image with link

* Add gitea copyright notice

* add a test for markdown image with link

* remove svg related variables
@lafriks lafriks added backport/done All backports for this PR have been created type/bug and removed type/enhancement An improvement of existing functionality labels Nov 8, 2018
techknowlogick pushed a commit that referenced this pull request Nov 8, 2018
* Fix markdown image with link

* Add gitea copyright notice

* add a test for markdown image with link

* remove svg related variables
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Markdown Image Hyperlinking Error Non-SVG image links don't link to intended website
6 participants