-
Notifications
You must be signed in to change notification settings - Fork 278
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
Issue#1919: Asset root path should be configurable #1933
Conversation
Added 'assetPath' variable to LiftRules, with default value '/assets'. Rest of the path structure is still convention based, not configurable. This will address the issue where the Lift template projects are storing the lazy loading gif under assets folder and the framework library expects it to reside at the root level. On the other hand, also if the default place for the assets is in the way of the application for a reason or another, it is now easier to move somewhere else. Added also my name to the contributors.md file, as instructed in the documentation.
Thanks for the contribution. Initial changes look good. I'll find some time to review in depth soon. |
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.
Looks reasonable, but I'm a bit concerned about whether or not we'll break applications that, to date, have placed their Ajax loader under /images
.
My inclination is that we should default this to /
and change the giter8 template to set it to /assets
so that the ajax loader works correctly there.
WDYT?
* follow conventions, for example the default lazy loading spinner | ||
* ('ajax-loader.gif') is under 'images' folder. | ||
*/ | ||
@volatile var assetPath: String = "/assets" |
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'm kind of torn over whether or not this default represents a breaking change or not. I'm inclined to say that we should default this to /
until Lift 4.0.
Open to other opinions, though.
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.
You're right, it would be a nasty surprise when upgrading to a new version of lift. I'll update the PR soon with the default path set to /. Thanks.
…Also the path must now end with "/" to denote a directory, rather that leave it open as it was before.
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.
This looks reasonable to me. Let's wait a bit for others to provide feedback. Barring anything in a few days I'll merge.
* the default lazy loading spinner ('ajax-loader.gif') is under | ||
* 'images' folder. | ||
*/ | ||
@volatile var assetPath: String = "/" |
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.
Would it make sense to have this default to ""
instead of removing the /
from the links?
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.
@eltimn I was thinking that option too, but eventually leaned towards this way. I was thinking that slash would make it clear that it is about root path, and also when with a folder value it would need to be there.
But, I'm happy with either way, just let me know. :)
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'm fine either way, really. I just wanted to bring it up and see if anyone else had an opinion.
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.
There is also a third possible approach, I wrote to the mailing list about it:
https://groups.google.com/d/msg/liftweb/bf07dlUfWpc/cUG6KXeiAQAJ
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'm down for the current approach. One thought though: should this be assetRoot
or assetPathRoot
?
Also, the scaladocs don't make it immediately clear that this is something that is relative to the domain root; that is, that this goes directly into HTML references and therefore /
means something other than “project root” or any number of other possibilities. It's also worth noting that this will in turn be rewritten to take context path into account, if that is set, for maximum fun.
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.
Yes, maybe it would be better to make those things more clear, the naming and also scaladoc. I'll try to find time to change them today after work.
I didn't merge this because it didn't seem like we've landed on an agreement yet. What are we thinking on how this should go? |
Oh sorry, I see the link about the ML. That conversation i still ongoing. We'll wait to see what happens. |
…se about where this variable effects.
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.
Believe this addresses my notes!
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.
lgtm
This would be super handy for splitting up a Lift app into modules behind an nginx proxy! |
Added 'assetPath' variable to LiftRules, with default value '/assets'. Rest of the path structure is still convention based, not configurable.
This will address the issue where the Lift template projects are storing the lazy loading gif under assets folder and the framework library expects it to reside at the root level.
On the other hand, also if the default place for the assets is in the way of the application for a reason or another, it is now easier to move somewhere else.
Added also my name to the contributors.md file, as instructed in the documentation.
Mailing List thread: