simogeo / Filemanager

An open-source file manager released under MIT license. Up-to-date for PHP connector. This package is DEPRECATED. Now, please use RichFileManager available at : https://github.com/servocoder/RichFilemanager.
937 stars 350 forks source link

Do not automatically load Filemanager and files. #197

Open Req opened 10 years ago

Req commented 10 years ago

This is a suggestion: Please use a config option and and initialization function to start Filemanager. Make the developer just load the one JS library and then let them decide when and how to instantiate Filemanager - this would include a custom configuration object. Path issues; a thing of the past (almost).

The config given during instance creation would be merged with a default config. You could even have the other JS and CSS files included by filemanager.js so you don't need to include them in the actual page. So, in the page with filemanager the dev could do something like this in index.html:

<script type="text/javascript" src="/my/path/filemanager.js"></script>
<script type="text/javascript">
var config = {
    basePath: '/my/folders/scripts/',
    onLoadSuccess: function() {},
    onLoadFail: function() { alert('Could not load filemanager!'); }
};
// use your favourite body.onload method for this
Filemanager.Create(config);
</script>

So Filemanager.create() would load all the dependancies as well taking the basePath into consideration, effectively replacing THIS 'url': './scripts/filemanager.config.js' with THIS: 'url': config.basePath + 'filemanager.config.js'

You could merge the config options with default values given during the creation phase with this (after jquery has loaded or by using the non-jq object concatenation)

// set default configuration options (example)
var config = $.extend({
    basePath: './scripts/',
    onLoadFail: undefined, // fail silently
    onLoadSuccess: function() { alert('Filemanager ready.'); }
}, userConfig);

Then when the backend config.js loads just merge that wit the userConfig too! This is so extensible too to throw good errors. Like onConfigLoadFail: function() {...} etc.

This would solve so many path issues and make integration and configuration easier. This method is great for page-specific configuration and event hooking (yes it's a real need). I would imagine that the connectors would be easier too after this, especially for sites and platforms that use complex routing. What you say? If you like it, when could this be implemented?

simogeo commented 10 years ago

Hi Req,

Thanks for you contrib. and for taking time for discussing it here.

I'm not opposed to your proposal but I actually have to admit that I'm bit sceptical and I explain you why :

This is my first impression and one more time I'm not closed to your proposition. It may take time to think a bit about it and implement it, if applicable.For now, I haven't much time.

If you feel like create a fork and implement this, just let me know. I'll follow your work and may integrate it later to the trunk, if I'm convinced with.

Req commented 10 years ago

Hello,

Thanks for your reply, this certainly clears the issue a lot. I was under the false impression that running FM as a full page would just be one option and not the core concept, so I was already wrong there. Without expanding the core idea behind FM there really isn't that much of a point to this. To me it limits the usability a little, but the change would be quite big indeed.

I was thinking of using FM as just one component on a page or being used within a view. That at least for ASP.net MVC3 projects is natural as they often have a common layout and the views are just elements within said layout. So, FM would similarly have been a DOM element with some (impressive) JS behind it :). I was thinking of something very much like what CKFinder does to be honest.

My misunderstanding was great and with the new information, this change might be a little too big.

Req commented 10 years ago

Oh BTW, the path issues I was talking about was not about the uploaded files folder path, although that is one issue too. I was referring to setting the path for where FM loads the the JavaScript files from (like filetree, impromptu, tablesorter) so I would not have to include them myself within the HTML markup like now. Having FM load the files instead of me is just for convenience and ease of integration.

I might use a path like /Scripts/Filemanager/scripts/filemanager.js - and I would have wanted to have different config options per page as well, so that was partly the origin of the path issues I had. One page would have been for administering PDF files within a document vault, one would have been for CKE and content images and so on, both would use the same core filemanager.js but they have their own filemanager.config.js files.

That way if Filemanager loaded the files itself and instantiates with a custom config I might have done something like this:

<div id="Filemanager"></div>
<script type="text/javascript" src="@Url.Content("/Scripts/Filemanager/scripts/filemanager.min.js")" />
<script>
$(function() {
    var config = {
        configPath: '/Scripts/Filemanager/scripts/pdfManagerConfig.js'
        scriptBasePath: '/Scripts/Filemanager/scripts/',
        domElement: '#Filemanager',
        onLoadSuccess: function() {},
        onLoadFail: function() { alert('Could not load filemanager!'); }
    };

    Filemanager.Create(config);
});
</script>

But I realize now that this would need a pretty major rewrite and would maybe mean for example skinning and massive changes because instantiating inside an element on a page means that the DOM for FM would be controlled purely by FM and stylesheets/config. I think a fork might be the best way to go here, but I don't have nearly enough time to actually start one - to quote Amir Blomenfeld; Oh sheesh y'all twas a dream.

simogeo commented 10 years ago

Indeed, it would make it a bit more flexible. But, as you, I don't have enough time to do so.

simogeo commented 10 years ago

You could merge the config options with default values given during the creation phase with this (after jquery has loaded or by using the non-jq object concatenation)

@Req : This is now implemented