lonnieezell / Bonfire

Jumpstart your CodeIgniter web applications with a modular, HMVC-ready, backend.
http://cibonfire.com
1.39k stars 524 forks source link

Do we need Template/Asset inerhitance? #805

Open lonnieezell opened 11 years ago

lonnieezell commented 11 years ago

@mwhitneysdsu @sourcejedi

Currently, both the Template library and the Asset library support inheritance. That is to say that the active theme can also look in the default theme (or another theme) to provide 'parent/child' type themes. This is a powerful feature. However, I have the feeling that it's one that no one uses, and might be causing more confusion than it should. Do we need to keep this feature in? Removing it would provide a small performance bump.

What are your thoughts?

mwhitneysdsu commented 11 years ago

This is actually one of the features that drew me to Bonfire in the first place, and one that I actively use quite heavily on my site. Besides my admin theme, I have 3 themes on my site, and most of my static content resides in the main theme. The other two themes primarily change some CSS/JS and make some changes to portions of the site's layout, but the content still resides in the main theme. For instance, our graduate admissions page (http://cbaweb.sdsu.edu/grad/admissions), which uses the site's main theme, pulls content from many different locations in the site. Some of those locations, like the Executive MBA site (http://cbaweb.sdsu.edu/emba/apply) and the Sports MBA site (http://cbaweb.sdsu.edu/smba/students/prospective#smba_admission) use a different theme.

This allows me to make a fairly broad set of changes to a set of pages by only adding/changing a line or two of code in the controller, once my themes are setup. In fact, many of our "sites" were built using the main theme, then moved over to another theme based on requests from the people that manage the various programs. (When I started this job, the College had ~20 websites in various locations, all using static pages; most of those websites are now integrated into cbaweb.sdsu.edu, but we often still refer to them as sites, and each site is often represented, in part, by a module in Bonfire).

Keeping the static content, templates, CSS, JS, images, and files in the themes and/or the assets directory also allows our graphics/web designer to handle the content in a directory structure she can understand without having to navigate the modules too often.

I think we could take better advantage of the asset inheritance (and make it more clear) by moving more of the core assets into the assets directory instead of having items like bootstrap in the themes. We also need to improve the code's consistency in calling un-minified asset filenames and allowing the asset library and configuration to determine when the minified files are utilized.

SethArchambault commented 11 years ago

I don't think I truly use the power of theme inheritance, but I have overtime found it useful to moving some of the bootstrap items from themes to the main assets directory.. So I guess I really don't use theme's in terms of Parent and child..

lonnieezell commented 11 years ago

@mwhitneysdsu That's awesome to hear! I had only heard the confusion it presented at times by people in the forums so thought it might have been getting in the way. Glad to hear it's been helpful.

I would love to start a discussion with you about the assets and themes portions to see what you think needs to happen and what would be your ideal situation so we can maybe start incorporating some of those ideas. In the reboot I had stripped out the template inheritance, which I now know is a 'bad idea'. :) However, I've taken some stuff from Rails 4 and have a really good start to their asset pipeline to work with better minification/combining flow, etc. I would need to talk that one out with someone, I believe, to figure out a good way to implement some of that stuff without breaking the existing sites.

mwhitneysdsu commented 11 years ago

I think we could always allow inheritance to be bypassed for those that won't use it.

I'll have to read up on the features of the asset pipeline in Rails 4, as it's not something I'm familiar with, personally.

I would really like to see some form of "named" assets to manage things like jQuery/jQuery-UI that usually have different file names from one version to the next. I would also like to see the assets library manage dependencies for scripts in some way, especially so we can remove the hard-coded jQuery call from the templates.

lonnieezell commented 11 years ago

To save you from reading all of their docs, here's what my reboot version currently supports:

https://github.com/lonnieezell/bonfire-reboot/wiki/Asset-Handling

I do think that providing an option to turn off inheritance is a good idea, since it would be a significantly smaller number of file-system hits. The other option might be to cache a file-map so we always know where they are...

mwhitneysdsu commented 11 years ago

Looking at just the documentation, I think the main issues you're going to run into with compatibility will be:

1) The order of the asset paths, which is documented in reboot as the opposite of the current order. The reason for the current order, as I understand it, is that themes and modules should take precedence, allowing overrides just as is done with the templates and modules themselves (so asset inheritance is predictable based on other inheritance behavior in the system).

2) The current optional arguments for add_js() and add_css() (though I believe in most cases add_css() is already broken in this respect). I think the primary issue here is going to be with inline js.

3) In some cases you may run into issues with add_js()/add_css() picking up module assets, which are currently only checked if the file was added through add_module_js()/add_module_css(). I personally think that, in the long term, putting the module path in the default search order would be a good thing; however, I think there are some outlying cases where the addmodule* methods would still be helpful (adding assets from modules other than the current module, for instance). There is also some slight potential that module assets would begin overriding base assets where they previously were not (but there are existing bugs in the addmodule* methods that prevent this from being likely, as the current code is only reliable if module assets are loaded in the controller's constructor).

Since most of the methods (other than add_js()/add_css()) use different names from those in the existing library, they could co-exist pretty readily for backwards compatibility for some time. The old methods could be marked as deprecated, and probably just act as a compatibility wrapper (translating arguments to the new methods or hooking into lower-level private methods).

lonnieezell commented 11 years ago

I've marked this as 0.8 so that we remember to check into for that. I won't do any changes for 0.7, but would like to take a fresh look at how the assets are handled for 0.8.