Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add support for loading CSS #76

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
6 participants

amrnt commented Jan 30, 2013

If you are going to use it from https://github.com/amrnt/basket.js, be sure to run grunt to build the new files in dist.

@wibblymat wibblymat and 1 other commented on an outdated diff Jan 30, 2013

lib/basket.js
@@ -115,7 +119,7 @@
promise = saveUrl( obj );
} else {
promise = new RSVP.Promise();
- promise.resolve( source.data );
+ promise.resolve( source.data, obj.key.search(/\.css$/i) !== -1 );
@wibblymat

wibblymat Jan 30, 2013

Collaborator

I would move this check into injectScript. Doesn't really seem to belong here.

Also, maybe injectScript needs renaming. 'addTo'DOM' or something like that.

Edit: Of course, you only currently get the text passed to injectScript so can't do the check there.

I made a change for my own PR in wibblymat/basket.js@6814adb which allows you to get the obj. It was done with exactly this in mind.

@amrnt

amrnt Jan 30, 2013

makes sense. addToDOM is a good replacement.

Yeah just found that I can't do the check there in injectScript function.

I think, I'm leaving as it is now, but change injectScript to addToDOM.

Contributor

felipemorais commented Jan 30, 2013

Could add some test?

amrnt commented Jan 30, 2013

@addyosmani @sindresorhus This is going to be bigger that this solution in the pull-request.

Previously, we considered that all the files we store are Javascripts, but now we have Stylesheets. It's hard to figure out if the file is javascript or stylesheet by the extension.

I suggest to append the key name of the asset that stored in the local storage with the content-type of the received data. So whatever the key is, it should end with -js or -css.

What do you think? I would like to continue working on implementing this.

Owner

addyosmani commented Jan 30, 2013

That's a valid concern. I wouldn't mind appending the keyname with the content-type received if it would help make this more feasible.

amrnt commented Jan 30, 2013

@addyosmani I've just push some changes. All current tests are still passing.

I didn't implement any tests. I tested it manually, and initially, it works as expected.

Owner

addyosmani commented Feb 4, 2013

Thanks for pushing the changes. We're going to spend some time stress testing the implementation and if all goes well we might request for tests to be added in order for us to merge this.

@sindresorhus @peol @wibblymat do you think this type of behavior should be in the project by default or added as a type of extension?

Contributor

sindresorhus commented Feb 4, 2013

I would think there is a reason Yepnope and RequireJS has it as an extension.

Collaborator

peol commented Feb 5, 2013

I kinda agree with @sindresorhus on this one.

We need another refactoring of the code base to be more generic. This adds more specific (css/js) logic into loads of places so my suggestion would be to generalize the whole flow to only store/fetch data/urls, and add in injection logic etc. as a plugin-type of deal. It'll increase maintainability a lot and we can then decide which plugins we'd support and which ones we'll leave up to the community. My $0.02.

No offense to you, @amrnt. Appreciate the work you've done here!

Collaborator

wibblymat commented Feb 5, 2013

Ultimately the only part of the code that cares what kind of data we have is the injectScript/addToDOM function. If we make it easy to override the behaviour of that function then it is trivial for people to make their own plugins and I have no problem with not baking in behaviour in the distribution for different types.

I imagine that you could add CSS support with something like:

basket.addHandler( 'text/css', function( obj ) {
    var el = document.createElement( 'style' );
    el.setAttribute( 'type', 'text/css' );
    if ( el.styleSheet ) { // >= IE6
        el.styleSheet.cssText = obj.data;
    } else { // Rest
        el.appendChild( document.createTextNode( obj.data ) );
    }
});

which is simple enough that you can just write it into the documentation if people want it.

Owner

addyosmani commented Feb 5, 2013

I actually like that approach. We could document them and perhaps also have a "plugins" directory for those wishing to just manually add support for these things.

Owner

addyosmani commented Feb 9, 2013

@wibblymat do you think we should implement a way to trivially add plugins to basket? If so, would you like to describe further API capabilities/what it's limitations would be? might be interesting to explore.

Owner

addyosmani commented Feb 13, 2013

@wibblymat I just wanted to let you know that I reviewed your proposal a few times and I really liked it. Thanks for writing it up! Let's discuss it further in early March once a few other projects get wrapped up :)

Contributor

sindresorhus commented Aug 17, 2013

Closing in favor of #84

@andrewwakeling andrewwakeling referenced this pull request Dec 26, 2013

Open

CSS Support #60

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment