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

Fixes Image Sources #459

Closed
wants to merge 3 commits into from
Closed

Conversation

manikmagar
Copy link
Member

Fixes #440 and #455

  1. Do not modify image source if it starts with http:// or https://.
  2. Prepend Resolved root path and page URL to image source, unless it starts with /. Eg. CompileOnSave.png -> ../../2011/05/CompileOnSave.png, ./CompileOnSave.png -> ../../2011/05/CompileOnSave.png but /CompileOnSave.png -> /CompileOnSave.png
  3. img.path.prepend.host (default true, otherwise it will not be backward compatible) decides if site.host should be prepended to image source with Point 2 above. When set to false, site.host will not be appended. [this along with addition root path should solve Option to generate relative image links in output #440 ]

@coveralls
Copy link

coveralls commented Apr 24, 2018

Coverage Status

Coverage increased (+0.1%) to 80.114% when pulling 375c4a2 on manikmagar:fix/issue455 into e72a7d8 on jbake-org:master.

@msgilligan
Copy link

What is "Resolved root path"?

@jonbullock
Copy link
Member

I'm guessing @manikmagar is referring to content.rootpath which is the path to the root of the project

Copy link
Member

@jonbullock jonbullock left a comment

Choose a reason for hiding this comment

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

Code looks good to me 👍

@manikmagar
Copy link
Member Author

Thanks @jonbullock. Yes I meant content.rootpath.


HtmlUtil.fixImageSourceUrls(fileContent, config);

String body = fileContent.get(Attributes.BODY).toString();

assertThat(body).contains("src=\"http://www.jbake.org/blog/2017/05/first.jpg\"");
assertThat(body).contains("src=\"http://www.jbake.org/../../../blog/2017/05/first.jpg\"");
Copy link
Member

Choose a reason for hiding this comment

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

hmm. I would have expected the url without the Attributes.ROOTPATH here.


assertThat(body).contains("src=\"http://www.jbake.org/../../../blog/2017/05/first.jpg\"");

Copy link
Member

Choose a reason for hiding this comment

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

same here. it's a strange looking url. as long as the site.host is present do we really need to append the rootpath here? the uri is relative to the website root already, isn't it?


String body = fileContent.get(Attributes.BODY).toString();

assertThat(body).contains("src=\"../../../blog/2017/05/first.jpg\"");
Copy link
Member

Choose a reason for hiding this comment

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

This will only work for the first_post.html file. if it is an post shown with content on the index page the url would be wrong openening the file directly from a browser. on the index page it should be blog/2017/05/first.jpg.

jonbullock added a commit to jonbullock/jbake that referenced this pull request Aug 9, 2018
Changed so root path isn't used anymore.
@jonbullock jonbullock added this to To do in v2.6.2 Release Aug 13, 2018
@jonbullock jonbullock moved this from To do to In progress in v2.6.2 Release Aug 13, 2018
jonbullock added a commit to jonbullock/jbake that referenced this pull request Aug 28, 2018
Changed so root path isn't used anymore.
jonbullock added a commit to jonbullock/jbake that referenced this pull request Aug 28, 2018
Changed so root path isn't used anymore.
jonbullock added a commit that referenced this pull request Aug 30, 2018
Merged in #459 and addressed code review suggestions - take 2
@jonbullock
Copy link
Member

Merged in manually as part of #535

@jonbullock jonbullock closed this Aug 30, 2018
@jonbullock jonbullock moved this from In progress to Done in v2.6.2 Release Aug 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants