stevie-mayhew / silverstripe-svg

Basic SVG support for SilverStripe
MIT License
22 stars 8 forks source link

HARVS1789UK Improvements #3

Closed HARVS1789UK closed 9 years ago

HARVS1789UK commented 9 years ago

Improved error catch when source SVG does not exist on file system, adding functionality to set a custom base path and sub-folders and allow for SVG name to optionally include a file suffix

stevie-mayhew commented 9 years ago

Hi @HARVS1789UK,

I have commented a couple of small changes to code and have a question about the addSubfolder code:

I'm not sure the addSubfolder functionality works as I would initially expect if you can define multiple subdirectories. If you were to define an SVG as

$SVG('my.svg').addSubfolder('MyDir').addSubfolder('MyOtherDir')

it would then find base_path/MyDir/MyOtherDir/my.svg, not look for the first match in either subfolder (which is how I would expect it to function). You could simply use

$SVG('my.svg').addSubfolder('MyDir/MyOtherDir')

and remove the need for them to be chainable in an array. At the moment you can achieve this functionality without using the new code all:

$SVG('MyDir/MyOtherDir/my.svg')

Do you think we need to add it?

Otherwise looks good, thanks for the contribution! :smile:

HARVS1789UK commented 9 years ago

Hi @stevie-mayhew,

I had not realised that it was possible to pass in a path relative to the base path to $SVG template logic, as all the example template code in the README.md and the property name in SVGTemplate both refer to this parameter/property as 'Name' instead of 'Path'.

Having looked back at the original code I can see your example of $SVG('MyDir/MyOtherDir/my.svg') would do the same.

However, (as well as making te ability to add sub-dirs a bit clearer) my method is also necessary in my context when using SVG's which have been uploaded to the CMS and are stored as records in the DB and in the /assets folder.

When using my custom base path method to set the base path to '/assets/Uploads/SVG' (or similar) there is no easy way to specify a sub-directory using the first parameter of $SVG as file objects only have properties of Name (e.g. MyImg.svg) or Filename (e.g. assets/Uploads/SVG/SomeSubfolder/MyImg.svg). Used in conjunction with my customBasePath() method neither of the following examples would work:

{$SVG($Image.Name).customBasePath('/assets/Uploads/SVG/') This would look for the file in /assets/Uploads/SVG/MyImg.svg and not find it

{$SVG($Image.Filename).customBasePath('/assets/Uploads/SVG/') This would look for the file in /assets/Uploads/SVG/assets/Uploads/SVG/SomeSubfolder/MyImg.svg and not find it

I suppose you could either do some concatenation of $SVG('SomeSubfolder' + $Image.Name) or set the custom base folder to be more specific i.e. $SVG($Image.Name).customBasePath('/assets/Uploads/SVG/'SomeSubfolder/) to facilitate for this requirement but now it's written I don't see much point in removing my method for adding a subfolder section to the base path.

It's up to you, I will certainly be leaving it in my codebase as I have already implemented it on a couple of sites that way, I can remove it from the pull request if you really wanted me to when I get a chance but I don't think it does any harm.

Kind regards,

HARVS1789UK

stevie-mayhew commented 9 years ago

That makes sense, I hadn't thought about that use case :smile:

stevie-mayhew commented 9 years ago

Thanks @HARVS1789UK

Great additions!