Skip to content

Fix markdown image with link#4675

Merged
techknowlogick merged 6 commits into
go-gitea:masterfrom
LER0ever:fix/md-image-links
Oct 30, 2018
Merged

Fix markdown image with link#4675
techknowlogick merged 6 commits into
go-gitea:masterfrom
LER0ever:fix/md-image-links

Conversation

@LER0ever
Copy link
Copy Markdown
Contributor

@LER0ever LER0ever commented Aug 12, 2018 •
edited
Loading

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.

techknowlogick
techknowlogick reviewed Aug 12, 2018
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

LER0ever commented Aug 12, 2018

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

techknowlogick commented Aug 12, 2018 

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

Needs to be added under the Gogs copyright

@LER0ever
Copy link
Copy Markdown
Contributor Author

LER0ever commented Aug 12, 2018

I see, fixed in fdd8ceb .

@codecov-io
Copy link
Copy Markdown

codecov-io commented Aug 12, 2018 •
edited
Loading

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

techknowlogick commented Aug 12, 2018

@LER0ever Thanks 😄

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

LER0ever commented Aug 20, 2018

@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
Closed
7 tasks
techknowlogick
techknowlogick reviewed Oct 30, 2018
View reviewed changes
Comment thread modules/markup/markdown/markdown.go
@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
lafriks
lafriks requested changes Oct 30, 2018
View reviewed changes
Comment thread modules/markup/markdown/markdown.go
@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
Copy Markdown
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
@LER0ever @lafriks
Fix markdown image with link (go-gitea#4675)
9d49e10 
* 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
@lafriks lafriks mentioned this pull request Nov 8, 2018
Merged
techknowlogick pushed a commit that referenced this pull request Nov 8, 2018
@lafriks @techknowlogick
Fix markdown image with link (#4675) (#5299)
996ce8c 
* 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.

Reviewers

@techknowlogick techknowlogick techknowlogick approved these changes

@lafriks lafriks lafriks approved these changes

Assignees

No one assigned

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

Milestone

1.7.0

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

@LER0ever @techknowlogick @codecov-io @lafriks @lunny @bkcsoft