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

Prevent image source path from breaking #367

Merged
merged 4 commits into from Aug 14, 2017

Conversation

manikmagar
Copy link
Member

This change prevents image source path from breaking. It prefixes
content rootpath to the image sources.

It excludes the sources that starts with http:// or https:// or /.

If image source starts with './' i.e. image is in the same folder as
content, then it will replace ./ with content directory path and then
adds the rootpath.

Once image source is fixed, it should not break, no matter from where
content is accessed i.e. index or individual page.

This change prevents image source path from breaking. It prefixes
content rootpath the image sources. 

It excludes the sources that starts with http:// or https:// or /.

If image source starts with './' i.e. image is in the same folder as
content, then it will replace ./ with content directory path and then
adds the rootpath.

Once image source is fixed, it should not break, no matter from where
content is accessed i.e. index or individual page.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 78.681% when pulling d02dc5f on manikmagar:image-src-path-fix into cde795e on jbake-org:master.

@jonbullock
Copy link
Member

Ah excellent, this addresses a common issue right now.

@manikmagar
Copy link
Member Author

Although this address image source issue while rendering posts/pages on the site,
I think still the image path in rss feed will be broken when viewed on external feed reader. Image sources will be relative.

@jonbullock do you think, we should make them fully qualified path instead of relative? eg. http://example.com/content/a.jpg instead or content/a.jpg?

@jonbullock
Copy link
Member

@manikmagar Yeah I agree we should make them fully qualified for the feed for this release. We can review this in subsequent releases.

@manikmagar
Copy link
Member Author

@jonbullock Feed just loads the content already parsed into HTML source code. To make feed paths working, we have two options -

  1. Use FQDN for everything, this will work when site is deployed to server but for local development, everytime jbake.properties will need to be updated to have localhost as site.host. Else, images will not load in local. I think, this might be inconvenient for development.
  2. After Feed is rendered, read the generated XML file to correct image sources in description content. Not sure, if this option is good.

Do you have any thoughts or suggestion?

@ancho
Copy link
Member

ancho commented May 31, 2017

👍 for full qualified domains. The ConfigUtil rewrites the site.host property when running with jetty. You need to run with -b option too. For a clean workflow, say running a server in background with -s while editing content, we would need a mechanism to detect a still running jetty instance launched from jbake to do the same thing.

@manikmagar
Copy link
Member Author

manikmagar commented May 31, 2017

When file is parsed, the rendered html content is stored in fileContents with key body. How about after fixing the body, we add 'feedBody' with FQDN's in it. Feed templates which now use body can use post.feedBody -

	<#escape x as x?xml>	
	${post.feedBody}
	</#escape>
	</description>

May be a simple fix? What do you think about this? @jonbullock

@jonbullock
Copy link
Member

I didn't fully understand the complexity of this when I suggested FQDN for the feed.

I'm undecided whether adding an additional variable is better or worse than post-processing the XML output to make image references use FQDN instead of just relative. Adding a new variable just for use in the feed seems overkill but post-processing a rendered file doesn't feel right either.

@manikmagar
Copy link
Member Author

manikmagar commented Jun 8, 2017

I was looking at img srcset attribute that was added with html5. It is used for the responsive image set, browser first tries to load image from srcset and if that is not available then it falls back to src attribute. I am thinking to add relative path in srcset and FQDN in src. Except feed, browser should always then use relative path and when feed is rendered in reader, FQDN would be used. I'll give it a try.

Replace rootpath with site host

Changed the implementation to prefix the site.host property before
images. Relative paths were breaking in feed.
@manikmagar
Copy link
Member Author

So, srcset did not work as expected. As @ancho said (btw thanks for that), ConfigUtil rewrites the site.host property to localhost when ran with -b -s together. So I changed the code to use FQDN always :).

In case of local baking, if -b -s is used, all image url's will point to localhost.

For releasing site, when just -b is used, site.host as specified in the jbake.properties will be used.

This should solve the feed problem too.

Eg.
Check the post at the bottom - On this feed

For site rendering - check this post (it is the same as in feed above)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 78.693% when pulling c4d32a7 on manikmagar:image-src-path-fix into cde795e on jbake-org:master.

@jonbullock jonbullock self-assigned this Jun 13, 2017
@jonbullock
Copy link
Member

Thanks @manikmagar any chance you can have a look at why the tests have failed after your latest commit?

@manikmagar
Copy link
Member Author

Ah! Sure. I will check and fix it.

Corrected JSOUP api method call to prevent empty body during test
execution. This would also prevent extra body element in parsed
fragment.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 78.693% when pulling 1a80c02 on manikmagar:image-src-path-fix into cde795e on jbake-org:master.

@manikmagar
Copy link
Member Author

Corrected JSOUP API method call to prevent empty body during test
execution. This would also prevent extra body element in parsed
fragment.

@jonbullock
Copy link
Member

Excellent thanks @manikmagar

@jonbullock
Copy link
Member

Linking mailing list discussion on this: https://groups.google.com/d/topic/jbake-user/64MiZRTl_3I/discussion

@jonbullock jonbullock merged commit 6d2f51d into jbake-org:master Aug 14, 2017
manikmagar added a commit to manikmagar/jbake.org that referenced this pull request Jan 14, 2018
jonbullock added a commit to jbake-org/jbake.org that referenced this pull request Jan 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants