-
Notifications
You must be signed in to change notification settings - Fork 326
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
Wraps image path update in config option #537
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes looks good to me. This will help those who do not wish to modify image urls. Also, backward compatible for those who are using it. Thanks!
@@ -283,10 +283,15 @@ | |||
boolean getUriWithoutExtension(); | |||
|
|||
/** | |||
* @return Flag indicating if image paths should be prepended with {@link #getSiteHost()} value | |||
* @return Flag indicating if image paths should be prepended with {@link #getSiteHost()} value - see {@link #getImgPathUpdate()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me this description is not clear enough. What different effects do I get with the different combinations of boolean values for img.path.prepend.host
and img.path.update
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
img.path.prepend.host
will have no effect when img.path.update
is set to false
.
When img.path.update
is true
, image paths will be modified to prepend with context.rootpath
value (see #459). Optionally, the site.host
value can also be prepended to modified image path. This site.host
modification is controlled by img.path.prepend.host
.
Hope that helps!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see context.rootpath
in https://github.com/jbake-org/jbake/blob/a108a6fb594fac567cbba3a3fd870ca9b1627c08/jbake-core/src/main/java/org/jbake/app/configuration/JBakeProperty.java nor an according getter in https://github.com/jbake-org/jbake/blob/a108a6fb594fac567cbba3a3fd870ca9b1627c08/jbake-core/src/main/java/org/jbake/app/configuration/JBakeConfiguration.java. But if that is the case, I think it is worth mentioning both properties in the javadoc (i.e. site.host and context.rootpath)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rootpath
variable is calculated for each source content file, which is why it isn't in the config classes, however that variable isn't used in this implementation anymore.
I'll update the javadoc though to make it clearer though. Thanks @kwin and @manikmagar 👍
Anyone else have any issue with this being merged in? |
Relates to #535 and #459