search

tagyoureit / nodejs-Pentair

An application to read/write to Pentair pool controllers.
14 stars 6 forks source link

Updated Bootstrap Implementation (WIP) #27

Closed arrmo closed 7 years ago

tagyoureit commented 7 years ago

Hey, really great improvement! I took a quick look and it's for sure headed in the right direction. I have a few nit-picky things, but I think we're at a point to push this whole thing to release 1.0.0. The one thing I think we should change is that it's best practice not to include other distributions as part of our package. We should install bootstrap with npm install bootstrap --save which will add it to the package.json and then automatically be installed by npm install. And looks like the same for bootswatch? I think you'll need to change your paths as NPM will install under node_modules\bootstrap, (and node_modules\bootswatch). You see any issue with this (besides a few extra themes installed)?

arrmo commented 7 years ago

Very good comments, agreed! Yes, let's move bootstrap to npm install - I like that! As for bootswatch, I removed it, but also ... let's think about how to add themes through npm?

I can change paths, no problem at all. Do you want to pull this in, release v1.0.0, and then make these updates? Or do it first? I'm good with either way, your call.

tagyoureit commented 7 years ago

Let's make the changes on this branch and then push out a clean v1.

arrmo commented 7 years ago

You bet, will do!

arrmo commented 7 years ago

BTW, should we do a similar thing for jQuery and jQuery UI?

arrmo commented 7 years ago

Pulling my hair out, trying to understand the link logic (to put to npm modules in client index.html). Dang it!

tagyoureit commented 7 years ago

I think for JQuery, since it is browser side only and we aren't including the files (just linking to them externally) we don't need to include it on the server side. https://www.npmjs.com/package/jquery seems to indicate it's fine to use the script tag.

To include bootstrap, this sounds like a feasible solution: http://stackoverflow.com/questions/27464168/how-to-include-scripts-located-inside-the-node-modules-folder

arrmo commented 7 years ago

OK, have it working (with bootstrap). Just let me know on jQuery / jQuery UI - my thinking is to do them the same, and then they are local also (better overall for access, right?). Once we conclude, I can push the update (but it's working now). Also ... expose Winston?

arrmo commented 7 years ago

OK, cleaned up, Bootstrap left to npm - and also generally cleaned up code. Look OK?

Thanks!