OpenBuildings / asset-merger

Managing and merging Assets
MIT License
72 stars 31 forks source link

Kohana 3.3 Port + YUI and a few other things. #12

Closed psaia closed 9 years ago

psaia commented 11 years ago

FIrst of all I want to say that this library has way more potential than the other options for Kohana, nice job. I really wanted to use it on a project so I've ported it over to Kohana 3.3 and added a few things. This has been working great for us but please let me know if I have missed anything.

With a real singleton it makes it easier to specify styles in the controller. For example I have something like this going on:

My controller:

// 'style' and 'scripts' could be specified in a parent class somewhere. 'dashboard'
// is page specific in this example.
$this->template->asset_groups = array('style', 'scripts', 'dashboard');

Assets::factory('dashboard')
    ->js('admin/js/plugins/underscore.js')
    ->js('admin/js/custom/dashboard.js');

My view template header:

<?php foreach($asset_groups as $group): echo Assets::factory($group); endforeach; ?>

Cheers

hkdobrev commented 11 years ago

All you have done is great! It's actually a tremendous amount of work.

I disagree with only two things. Which might not be a stopper.

You could have done this with opening different issues and pull requests and discussing it all.

And the other thing is that the factory method you use should be called instance. There is a Singleton pattern and there is a Factory pattern. Here you have used the Singleton pattern and therefore it should be Assets::instance().

Examples for the Singleton pattern in Kohana are

Examples for the Factory pattern are:

A Kohana singleton style factory method

This sentence just wasn't making sense for me.

@ivank Anything to add?

psaia commented 11 years ago

Fair enough. I apologize about not breaking these into smaller issues. Feel free to decline and handle this in-house, as the documentation would need to be updated as well. You raise a very good point with the way the factory method is implemented. I agree that instance is much more appropriate and conventional. I should have put some more thought in to that during the migration. In my most recent commit I addressed this issue. 5eb67591e7b077cd45c231a7337d24b1aef0acc9 I was actually more worried that I was missing the bigger picture as to why a factory pattern was used. However, for what it's worth this pattern fits my needs well. Thanks for the feedback and I hope this helps.