Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CommonJS/RequireJS compatibility, Bower/NPM #6

Merged
merged 12 commits into from May 16, 2015
Merged

CommonJS/RequireJS compatibility, Bower/NPM #6

merged 12 commits into from May 16, 2015

Conversation

IngwiePhoenix
Copy link
Contributor

  • bootstrap.native can be require()'d now, and if loaded via a <script ...> tag, it just turns back to it's old behaviour. In an environment without window being defined, a factory function is returned. Otherwise, the actual plugin.
  • When you cd into the folder, you can create minified versions using Gulp. The dist/ folder contains the latest snapshot.
  • Bower and NPM are now supported too. their respective .json files should be correct.

To use gulp:

$ cd bootstrap.native
$ npm install
$ npm run gulp

And this will create a new distribution from lib/ into dist/.

@thednp
Copy link
Owner

thednp commented May 16, 2015

Would you please test this all and also edit the README for usage with all these things as well :D

@thednp
Copy link
Owner

thednp commented May 16, 2015

Hold your horses for a second. Please consider the following:

Example, bower.json should have the following changes:

  • "description": "Bootstrap 3's JavaScript re-implemented in vanilla JS! No jQuery required, at all.", should be "description": "Native Javascript for Bootstrap 3, the sweetest Javascript library without jQuery.",
  • "keywords": [ "bootstrap.native", "bootstrap", "vanilla.js" ], should include native so please make it "keywords": [ "bootstrap.native", "bootstrap", "vanilla.js", "native javascript" ],

Another thing I noticed, for instance package.json

  • "homepage": "https://github.com/thednp/bootstrap.native#readme" should be "homepage": "https://thednp.github.io/bootstrap.native/"

UPDATE: About versions: if no change has been made into the functionality of the code, except for the module implementations, bootstrap.native must have 0.9.7 tag.

UPDATE: One last thing: do the bower/require/etc implementation credits in the README, and not in all the bootstrap native js files or leave that to me.

UPDATE: Please don't move the scripts (eg. affix-native.min.js files) to dist folder, leave them where they are, that's where CDN bot takes new releases from, so it may be a problem. And I don't think they need to be there, the CDN is good enough to have bootstrap-native.js and bootstrap-native.min.js, IMO. BUT, if you have a better argument, make sure to let me know.

If you're working locally, you can do a quick find replace, but please test everything twice so I make sure our next release (I don't know such stuff and cannot test it), you must know the CDN files must be perfect.

Thanks for your attention :)

@IngwiePhoenix
Copy link
Contributor Author

Hey.

Sorry, I must've been getting a little over-excited. o-o;

I did all the tests locally - from Gulp, to the dist, to the RequireJS and such, all the way up. I wanted to make sure that the PR is complete. :)

I'll fix the Readme and *.json's in a little. The changes should automatically appear on this PR too sicne it'll go into master. That also means I'll revert README back to the original.

Once I commited that, I'll let you know!

Kind regards, Ingwie

PS: You are very welcome!

@IngwiePhoenix
Copy link
Contributor Author

oH i just noticed that I had a misunderstanding in the part of the readme. I'll add that in. In fact, I actually put it into index.js as a doc-comment... Derp! :)

@thednp
Copy link
Owner

thednp commented May 16, 2015

Please check my updates to the above comment.

FYI: I'm still updating that comment with further instructions, so please stick to it.

@IngwiePhoenix
Copy link
Contributor Author

I kinda did the doc-comment out of habbit. x) I'll fix that up for sure.

Abut the *.min.js files. I moved them into dist, because minified files usually are distributables, while the original JS files within lib/ are, well, the actual code. It just makes a bit more sense, since the minified versions are what is distributed to the client (= dist). Besides that, the Gulp task I wrote takes all the JS files from the lib folder and minifies them - i thought that'd be a distribution step.

However, if you prefer, I can change that behaviour to write bootstrap-native.min.js (currently bootstrap.native.min.js because I hit the dot instead of the dash) and its unminified friend to dist/ as before, but writing all the other minified files back into lib/min/. That decision is up to you. I'll wait on your word on this one before I do any change there.

@thednp
Copy link
Owner

thednp commented May 16, 2015

The file naming must remain the same for /dist folder(s), it's about CDN, I'm not 100% sure about it, I just wanna make sure everything is perfect before anything, anytime and everyone.

@IngwiePhoenix
Copy link
Contributor Author

Here you are! :) Pushed my changes as requested.

  • Changed the README to properly introduce the CommonJS/RequireJS usage and made extense notice about the factory functions. In fact, this method for changing the key values comes from jQuery itself.
  • Updated the *.json files for NPM and Bower correctly with the description, keywords and alike. I just need to make the registries aware of the change in the files...but that is no biggie.
  • I did keep the doc-comment in all.js on the root level. If you want that remove, I can do that too or you can remove it after merging.
  • Gulp now writes the minified library files back into lib/min/ as the current upstream has, while properly dropping bootstrap-native(.min).js into dist/.

I also have a 0.9.7 tag on my local repo, but I have a feeling that this isn't popping up in the PR...

@thednp
Copy link
Owner

thednp commented May 16, 2015

Thank you, I'll check your repo and post here any comments. You should NOT worry about versioning in terms of my code, you should only check the versioning of your own NPM/REQUIRE/Bower implementation stuff.

@thednp
Copy link
Owner

thednp commented May 16, 2015

I want you to know, I have no problem having your name in the files, just that adding all people's name in the file would create a huge list overtime. Makes sense to have a list of contributors in the README, or a simple link to repository contributors :)

@IngwiePhoenix
Copy link
Contributor Author

Gotcha. :) Just wasn't sure about a good aproach to leave a tiny trail ^^ Gonna add that in.

@thednp
Copy link
Owner

thednp commented May 16, 2015

No need to be sad about my decision on naming, I think it's fair for all.

@thednp
Copy link
Owner

thednp commented May 16, 2015

Would this stuff require work with CDN files as well? Does it have an on(error) handling? Sometimes files don't load. If so, it would be great to be able to fallback to local distribution.

@IngwiePhoenix
Copy link
Contributor Author

It does automatically fall back. Take a look into the files themselves. If module is not defined at all, it'll return to what your original implementation was entirely.

Besides, the on("error") event should be given by the module system in question. Though, there should be no problem. I did not run across any in my testing.

@thednp
Copy link
Owner

thednp commented May 16, 2015

Please add YOUR name to all your files, example below, I will delete it from my files after merge, as said above.

// Bootstrap Native Javascript
// Bower/npm/Require implementation by Your Name

@IngwiePhoenix
Copy link
Contributor Author

Ohh, I get it. I only added "Edited by". Let me fix that into a proper line.

@IngwiePhoenix
Copy link
Contributor Author

Now it reads:

// Native Javascript for Bootstrap 3 | Affix
// by dnp_theme
// Bower/NPM/Require implementation by: Ingwie Phoenix

That this one look right?

@thednp
Copy link
Owner

thednp commented May 16, 2015

It's alright my friend, just your name should be in your own files only.

@IngwiePhoenix
Copy link
Contributor Author

Well I did modify every file in lib/. So... should I add my name in them too? I did that on my local copy for the moment.

@thednp
Copy link
Owner

thednp commented May 16, 2015

All YOUR files should have YOUR name credits (if you include me, it's ok, but not required), my files keep my name only.

@IngwiePhoenix
Copy link
Contributor Author

Alright, got it. Will push the commit in a sec.

Also sorry if I confuse things; english is not my native language. I am actually german. ^^

@thednp
Copy link
Owner

thednp commented May 16, 2015

No rush, take your time, test everything twice.

@thednp
Copy link
Owner

thednp commented May 16, 2015

If you care to put your name on it, I will let you know a much better way.

@IngwiePhoenix
Copy link
Contributor Author

Oh it's fine. :) There you are. The code is ready to merge. If you have anything else on your mind, let me know.

@thednp
Copy link
Owner

thednp commented May 16, 2015

OK now, last thing: please consider editing the html files, write a new section in the Use chapter, about using your stuff, later I will write a big thanks in your name, and this is gonna be visible in the demo and download pack.

UPDATE: take your time, no need to rush. It's @#$% week-end :)

thednp added a commit that referenced this pull request May 16, 2015
CommonJS/RequireJS compatibility, Bower/NPM
@thednp thednp merged commit fea322d into thednp:master May 16, 2015
@IngwiePhoenix
Copy link
Contributor Author

I will! I am just going to do a last confirmation, and give out the HTML fix. I also spotted a little issue in my exporting function. Those, however, are fixed in less than a minute.

You are very welcome! I'll be happy to use this in my project - and it is going to help me a lot!

@thednp
Copy link
Owner

thednp commented May 16, 2015

Alright, let me know when this is all tested, can you please provide some screenshots? I need to know this works perfect before I push a new version to the CDN.

@thednp
Copy link
Owner

thednp commented May 16, 2015

You can check my new project README :)

@thednp thednp mentioned this pull request May 16, 2015
@thednp
Copy link
Owner

thednp commented May 16, 2015

Alright, I've made some changes to your files, checking everything, and waiting for your fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants