igaster / laravel-theme

Theme support for Laravel
MIT License
520 stars 113 forks source link

Change "vendor-as-root" to "namespace-overrides" #35

Closed hailwood closed 8 years ago

hailwood commented 8 years ago

Hey @igaster,

Sorry to be a pain with all these pull requests but I'm just trying to give back because this package is awesome :+1:

Given the quantity of features I'm adding I might have a go at writing up some tests next week for us.


So this first change replaces the last addition I added -

The last addition allowed you to specify that if the $namespace (namespace::view.name) was in the vendor-as-root array it would render it from the root theme views folder.

This allows you to instead specify overrides for where the namespaces should render to which allows for the old behaviour but also something a lot more useful.

e.g.

// /views/ in the below examples is the path to the active themes views directory
[
    'namespace-overrides' => [
        'ns-a' => ''           // view('ns-a::some.view') = /views/some/view.blade.php
        'ns-b' => 'modules'    // view('ns-b::some.view') = /views/modules/some/view.blade.php
        'ns-c' => 'sub/folder' // view('ns-c::some.view') = /views/sub/folder/some/view.blade.php
        //ns-d not in array so // view('ns-d::some.view') = /views/vendor/ns-d/some/view.blade.php
    ]
]

This second commit just tidies up the file as there was mixed tabs and spaces which was making editing it a pain.


The third commit adds firing an event when the theme changed so we can now do specific actions if our theme is set to be active.


The fourth commit fixes the custom vendor dir being added multiple times due to incorrect usage of Arr::has

We'll need to update the readme, But you're better than me at that so I'll leave that task to you.

codecov-io commented 8 years ago

Current coverage is 39.03%

Merging #35 into master will decrease coverage by 0.52%

@@             master        #35   diff @@
==========================================
  Files             6          6          
  Lines           225        228     +3   
  Methods          41         41          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits             89         89          
- Misses          136        139     +3   
  Partials          0          0          

Powered by Codecov. Last updated by d1b504b...d8a42b1

igaster commented 8 years ago

These are very nice additions... Just one notice: It seems that 'namespace-overrides' dublicates the previous 'vendor-as-root' functionality. Can you please merge them in one single option?

Maybe when you pass a simple array it will defaults to the 'vendor-as-root' functinality, and when you set a key-value pairs array it will allow you to specify custom paths. (Just a suggestion). In case of Breaking Changes from the previous commit I can switch to a 1.2.x version

hailwood commented 8 years ago

Hey @igaster,

You are correct in that is does duplicate the previous functionality - this commit actually stops that working but the same functionality can be accomplished by passing through a falsy value as the value (e.g. '' as shown in the first example).

To stop it being a breaking change I could add the below block below the line $namespaceOverrides = $this->themeEngine->config('namespace-overrides', []);

foreach($this->themeEngine->config('vendor-as-root', []) as $rootVendor){
    $namespaceOverrides[$rootVendor] = '';
}

But I didn't originally as I wasn't sure if we wanted too bloat the codebase a little to support a feature that had been around less than a week.

I'm happy to add it in though - your call.

I was thinking about supporting a single dimensional array for the vendor-as-root type functionality but I wasn't sure if the additional complexity was worth add to save someone writing => ''

igaster commented 8 years ago

No it's OK. Keeping it simple is better than confusing with many details. Also 'namespace-overrides' explains better it's purpose and is easier to understand... I will commit it and delay a new version for 2-3 days in case you need to make any changes...

Thank you for your contributions!

igaster commented 8 years ago

Also updated the readme. You can check me... Cheers!