Fix Javascript Class/Driver Documentation (#17) #2001

Closed
wants to merge 8 commits into
from

Projects

None yet

3 participants

@BillHeaton

Brings the documentation back to a consistent, tested, working baseline
for the code. No Code changes this pass so may be suitable for current
version of CI.

Note: First Pull Request; Using Mac github and couldn't find howto set signoff in UI, please advise.

@BillHeaton BillHeaton Fix Javascript Class/Driver Documentation (#17)
Brings the documentation back to a consistant, tested, working baseline
for the code. No Code changes this pass so may be suitable for current
version of CI.
c909918
@narfbg narfbg and 1 other commented on an outdated diff Nov 15, 2012
user_guide_src/source/libraries/javascript.rst
-Initializing the Class
-======================
+Configuration
+=======================
@narfbg
narfbg Nov 15, 2012 Contributor

These should have the same length as the title itself.

Edit: Please also keep the initialization section - every library and/or helper has it.

@BillHeaton
BillHeaton Nov 16, 2012

ACK

On Nov 15, 2012, at 7:41 AM, Andrey Andreev wrote:

In user_guide_src/source/libraries/javascript.rst:

-Initializing the Class
-======================
+Configuration
+=======================
These should have the same length as the title itself.


Reply to this email directly or view it on GitHub.

@BillHeaton
BillHeaton Nov 16, 2012

Two paragraphs down. I'll change the label back to "Initializing the Class" does it need to be shifted back up?

@narfbg
narfbg Nov 16, 2012 Contributor

It should be consistent with other libraries, starting with a brief library description and how to load it.
See e.g. user_guide_src/source/libraries/image_lib.rst

@narfbg narfbg and 1 other commented on an outdated diff Nov 15, 2012
user_guide_src/source/libraries/javascript.rst
-To initialize the Javascript class manually in your controller
-constructor, use the $this->load->library function. Currently, the only
-available library is jQuery, which will automatically be loaded like
-this::
+To use a Javascript library you will need to set a configuration item for the path to the Javascript library that you'll be using.
@narfbg
narfbg Nov 15, 2012 Contributor

Could you change long lines so that they have between 70 and 80 characters per line?

@BillHeaton
BillHeaton Nov 16, 2012

ACK
On Nov 15, 2012, at 7:42 AM, Andrey Andreev wrote:

In user_guide_src/source/libraries/javascript.rst:

-To initialize the Javascript class manually in your controller
-constructor, use the $this->load->library function. Currently, the only
-available library is jQuery, which will automatically be loaded like
-this::
+To use a Javascript library you will need to set a configuration item for the path to the Javascript library that you'll be using.
Could you change long lines so that they have between 70 and 80 characters per line?


Reply to this email directly or view it on GitHub.

@narfbg narfbg and 1 other commented on an outdated diff Nov 15, 2012
user_guide_src/source/changelog.rst
@@ -446,6 +446,7 @@ Bug fixes for 3.0
- Fixed a bug (#1220) - :doc:`Profiler Library <general/profiling>` didn't display information for database objects that are instantiated inside models.
- Fixed a bug (#1978) - :doc:`Directory Helper <helpers/directory_helper>` function :php:func:`directory_map()`'s return array didn't make a distinction between directories and file indexes when a directory with a numeric name is present.
- Fixed a bug (#777) - :doc:`Loader Library <libraries/loader>` didn't look for helper extensions in added package paths.
+- Fixed Doc Errors (#17) - :doc:`Javascript Library <libraries/javascript>` Updated document to reflect working code.
@narfbg
narfbg Nov 15, 2012 Contributor

This should be removed.

@BillHeaton
BillHeaton Nov 16, 2012

Confirm? Document changes don't go in the change log?

On Nov 15, 2012, at 7:43 AM, Andrey Andreev wrote:

In user_guide_src/source/changelog.rst:

@@ -446,6 +446,7 @@ Bug fixes for 3.0

  • Fixed a bug (#1220) - :doc:Profiler Library <general/profiling> didn't display information for database objects that are instantiated inside models.
  • Fixed a bug (#1978) - :doc:Directory Helper <helpers/directory_helper> function :php:func:directory_map()'s return array didn't make a distinction between directories and file indexes when a directory with a numeric name is present.
  • Fixed a bug (#777) - :doc:Loader Library <libraries/loader> didn't look for helper extensions in added package paths.
    +- Fixed Doc Errors (#17) - :doc:Javascript Library <libraries/javascript> Updated document to reflect working code.
    This should be removed.


Reply to this email directly or view it on GitHub.

@BillHeaton
BillHeaton Nov 16, 2012

I've commented the changes you requested. Do I need to do another pull request? Thanks, BIll

On Nov 15, 2012, at 7:43 AM, Andrey Andreev wrote:

In user_guide_src/source/changelog.rst:

@@ -446,6 +446,7 @@ Bug fixes for 3.0

  • Fixed a bug (#1220) - :doc:Profiler Library <general/profiling> didn't display information for database objects that are instantiated inside models.
  • Fixed a bug (#1978) - :doc:Directory Helper <helpers/directory_helper> function :php:func:directory_map()'s return array didn't make a distinction between directories and file indexes when a directory with a numeric name is present.
  • Fixed a bug (#777) - :doc:Loader Library <libraries/loader> didn't look for helper extensions in added package paths.
    +- Fixed Doc Errors (#17) - :doc:Javascript Library <libraries/javascript> Updated document to reflect working code.
    This should be removed.


Reply to this email directly or view it on GitHub.

@narfbg
narfbg Nov 16, 2012 Contributor

No, but you do need to push your local changes to your fork on GitHub in order for the pull request to be updated.

@BillHeaton BillHeaton Fix Javascript Class/Driver (#17) - Cleanup
Corrected Formatting Issues. Removed line from ChangeLog.
8309596
@pickupman

Probably should change the protocol syntax for the CDN.
//ajax.googleapis.com/ajax/libs/jquery/1/jquery.min.js

This would allow the latest 1.x minified release and would support both http(s) pages. Right now SSL will fail using http to access the resource.

Owner
@narfbg narfbg and 1 other commented on an outdated diff Nov 18, 2012
user_guide_src/source/libraries/javascript.rst
-Set these variables in your view
---------------------------------
+Load the library
+----------------
+Initialize the Javascript class in your controller constructor, for example:
@narfbg
narfbg Nov 18, 2012 Contributor

Please don't tell people where to initialize a library when it is not an absolute must - it's their choice.

Also, your version of the doc states that the developers need to set a configuration, but you're not showing them how to pass it to the library.

@BillHeaton
BillHeaton Nov 18, 2012

Ack on the Initialize.

Ack on configuration. Expanded the reference to the Config library documentation so that it points out that your can do three ways.

On Nov 17, 2012, at 4:10 PM, Andrey Andreev wrote:

In user_guide_src/source/libraries/javascript.rst:

-Set these variables in your view

+Load the library
+----------------
+Initialize the Javascript class in your controller constructor, for example:
Please don't tell people where to initialize a library when it is not an absolute must - it's their choice.

Also, your version of the doc states that the developers need to set a configuration, but you're not showing them how to pass it to the library.


Reply to this email directly or view it on GitHub.

BillHeaton added some commits Nov 18, 2012
@BillHeaton BillHeaton Fix Javascript Class/Driver Documentation (#17) (#2001)
Don't specify where to initalize class.  More detail on how to set the
config item.
21413e7
@BillHeaton BillHeaton Fix Javascript Class/Driver Documentation (#17) (#2001)
Cleanup long lines. Sigh!
385a6ee
@BillHeaton BillHeaton Fix Javascript Class/Driver Documentation (#17) (#2001) pass 3
Travis build failed; Was a module that I hand not changed.  Not
apparent how to run.
a845c73
@narfbg
Contributor
narfbg commented Nov 22, 2012

Still not looking good enough ... the initial "Setup and configuration" was good, why do you need to change it to something related to controllers at all?

@BillHeaton

Andrey, Is this some sort of hazing ritual? I've updated my github profile so that you have a way to obtain a better idea of my abilities.

The original "Setup and Configuration" was mediocre at best. It presented the information in an order that was difficult to follow. It combined two disparagement topics, contained misinformation, and ambiguous, superfluous statements.

Simply saying: "Still not looking good enough" is an unfair criticism as your not giving me anything to fix. However I will address your comment: What I have submitted is information that is structured in a coherent, easy to follow, logical order for someone that is attempting to implement the code in their application.

In regards to your remark about "Controllers": In the MVC pattern, it is the logical place to initialize, add the elements, and to compile() and follows conventions that are used extensively in other parts of the framework. The only other place that perhaps makes sense in the MVC pattern is in a model; not useful in the scope of the examples I'm using.

The intent of the commit was to create a baseline of the documentation that would allow users to begin to use the class. The sooner we finish this commit the soon that I can move on to some of the egregious bugs that are stopping this class from being a valuable part of the CI framework; currently it is less than useful.

It is important to get the base correct because beyond jquery there is a need for jquery-ui and jquery-mobile elements that are easily accessable from CI.

@narfbg
Contributor
narfbg commented Nov 22, 2012

Fair enough. I'm sorry if you feel that my comments are inappropirate and you're right - they should've been more descriptive.

On a side note - I'm not seeing anything relevant on your github profile, although that probably is due to github not actually showing everything that you're able to input. However, I never intended to question your abilities, so that doesn't matter - you shouldn't have taken this personally.

Now to the point - I was talking about the section title in specific, not that the original content for "Setup and configuration" was better. Ideally, we should have a section titled similar to "Setup and configuration" that describes your current "Configuration" section and the $this->load->library() calls, including the old example of how to pass the configuration directly to the loader, e.g.:

$config = array(
    'javascript_location' => 'http://ajax.googleapis.com/ajax/libs/jquery/1.8.2/jquery.min.js'
);

$this->load->library('javascript', $config);

This, IMO is way more straightforward than references to the Config class, which isn't required at all. It is a good reference indeed (and should be kept), but not mandatory.
Btw, another misc. note on your configuration section - please don't "ucfirst" file system paths, they are case-sensitive on pretty much all UNIX-based OSes and our directory is 'application', no capital letters.

On why I question mentioning the controller, while I do agree on your points on the MVC concepts - CodeIgniter has never enforced this structure and everybody is free (although discouraged) to even call this from a helper, if they wish. So while it is obviously a requirement to echo $library_src and $script_head in the view, the rest of it should simply be titled e.g. Usage, not controller code.
I could agree with you on this, just showing my thoughts.

On adding jquery-ui, jquery-mobile, etc. While I'm not sure what the current status of Sparks is, you should take a look at issue #212. Some existing libraries like Cart and Javascript don't add much to the value of CI itself and would be better off as external components (addons, plugins, packages, whatever you wish to call them).
For historical reasons, they will be a part of the framework until we have a solution to this problem, but I just can't tell if that would be soon or not.

@BillHeaton

Ahhh.... That's makes more sense... Do we have a template for the documentation anywhere? It would be wonderful for when new people show up. Been coding for 30+ years so show me what you want and I can deliver.

I didn't take it personally, been around along enough that I quack like a duck and let it slide off my back. The pushback was about the quality of the documentation. I feel it's important the somebody unfamiliar with the class should be able to get a minimal implementation working with a reasonable amount of effort.

BTW: How do we discuss proposed changes to the code?

As someone pointed out the url to the Google CDN should not have a protocol. i.e. It should be "//ajax.googleapis.com/ajax/libs/jquery/1.8.2/jquery.min.js" which works with both http: and https: I'd also like to make that url the fallback default so that newbies don't get stymied. Fixing that routine would also knock out another issue.

Bill - http://www.winkwinknodnod.net/about

On Nov 22, 2012, at 5:41 AM, Andrey Andreev wrote:

Fair enough. I'm sorry if you feel that my comments are inappropirate and you're right - they should've been more descriptive.

On a side note - I'm not seeing anything relevant on your github profile, although that probably is due to github not actually showing everything that you're able to input. However, I never intended to question your abilities, so that doesn't matter - you shouldn't have taken this personally.

Now to the point - I was talking about the section title in specific, not that the original content for "Setup and configuration" was better. Ideally, we should have a section titled similar to "Setup and configuration" that describes your current "Configuration" section and the $this->load->library() calls, including the old example of how to pass the configuration directly to the loader, e.g.:

$config = array(
'javascript_location' => 'http://ajax.googleapis.com/ajax/libs/jquery/1.8.2/jquery.min.js'
);

$this->load->library('javascript', $config);
This, IMO is way more straightforward than references to the Config class, which isn't required at all. It is a good reference indeed (and should be kept), but not mandatory.
Btw, another misc. note on your configuration section - please don't "ucfirst" file system paths, they are case-sensitive on pretty much all UNIX-based OSes and our directory is 'application', no capital letters.

On why I question mentioning the controller, while I do agree on your points on the MVC concepts - CodeIgniter has never enforced this structure and everybody is free (although discouraged) to even call this from a helper, if they wish. So while it is obviously a requirement to echo $library_src and $script_head in the view, the rest of it should simply be titled e.g. Usage, not controller code.
I could agree with you on this, just showing my thoughts.

On adding jquery-ui, jquery-mobile, etc. While I'm not sure what the current status of Sparks is, you should take a look at issue #212. Some existing libraries like Cart and Javascript don't add much to the value of CI itself and would be better off as external components (addons, plugins, packages, whatever you wish to call them).
For historical reasons, they will be a part of the framework until we have a solution to this problem, but I just can't tell if that would be soon or not.


Reply to this email directly or view it on GitHub.

@narfbg
Contributor
narfbg commented Nov 23, 2012

I don't oppose excluding the protocol part of the URL. The way I understand it - in this current case it's just a matter of whether the developer includes it while doing their configuration, so what's the problem with it?

Proposed code changes are usually discussed on the pull request containing the code in question, but if you mean something more in terms of planning - if you feel that something needs to be changed, you can open an issue and explain your concerns. Only this wouldn't be appropriate for small changes - being able to look at the 10 lines changed in a diff is usually way better. :)

For this current PR, I've got no more suggestions than those explained in my previous comment. :)

@pickupman
Contributor

Glad to see this finally getting taken care of. I had authored these changes back in Feb 2011 (#17) before the issues where ported over from bitbucket. The other issue in my changesets was the proper naming of the child drivers.

According to the user guide the proper folder structure should be:
/system/libraries/Javascript
/system/libraries/Javascript/javascript.php
/system/libraries/Javascript/drivers/Javascript_jquery.php

See changeset:
https://bitbucket.org/pickupman/codeigniter-reactor/changeset/5ea945c28a83

@narfbg In order to meet the proper code formatting changes to match the proper documentation for a driver the syntax needs to be updated as such. I am sure my changesets would be somewhat out of date, but I could reauthor any changes. Thoughts?

For some reason the javascript library is been quite elusive, and under scrutiny. I believe this partly to do with the codebase likely in ExpressionEngine for backwards compatibility..

@narfbg
Contributor
narfbg commented Nov 26, 2012

@pickupman Go for it! I don't see many people enthusiastic about dealing with the Javascript library (even if it's broken), so any fix would be welcome.

@BillHeaton

@pickupman, I want to tackle the external() next. It can't handle protocol-less urls which is very useful for CDNs. I'd also like the default be to be the google CDN but don't know if that'l raise objections.

I have your changeset open in the other window, and I like what I see. At a minimum that starts to look like a driver to me.

However the devil's advocate in me jumps up and frantically waves it's hand to say : "Does it really want to be a driver?" My interest doesn't go far beyond the jQuery Family. Yes, I know there are other families of Javascript; but I wonder how many people would mix 'n match.

There's just something about putting a stub in for each function that bothers me; and also don't like the: "$this->javascript->jquery->blah()" method. Probably adds up my wanting to have my cake and eat it too.

Now that I've said all that; I'm doing a major update to my admin backend and I'm perfectly willing to go down the driver path and see how it holds up.

On Nov 26, 2012, at 1:42 PM, pickupman wrote:

Glad to see this finally getting taken care of. I had authored these changes back in Feb 2011 (#17) before the issues where ported over from bitbucket. The other issue in my changesets was the proper naming of the child drivers.

According to the user guide the proper folder structure should be:
/system/libraries/Javascript
/system/libraries/Javascript/javascript.php
/system/libraries/Javascript/drivers/Javascript_jquery.php

See changeset:
https://bitbucket.org/pickupman/codeigniter-reactor/changeset/5ea945c28a83

@narfbg In order to meet the proper code formatting changes to match the proper documentation for a driver the syntax needs to be updated as such. I am sure my changesets would be somewhat out of date, but I could reauthor any changes. Thoughts?

For some reason the javascript library is been quite elusive, and under scrutiny. I believe this partly to do with the codebase likely in ExpressionEngine for backwards compatibility..


Reply to this email directly or view it on GitHub.

@pickupman
Contributor

@BillHeaton I agree about your points for the necessity of a driver. jQuery has somewhat become the standard, but I suppose someone could write a driver for another framework or syntax. I like the idea of it be framework agnostic.

I have put up a commit including your commits to the docs and changelog. Here's my latest changeset:
pickupman@c8267a1

I can figure out if this file path issue in Windows, but the code in the changeset should be working, but the Loader class is setting a blank lib_name in Driver::load_driver(). Since lib_name is blank, I am unable to call the adapter methods.

@narfbg
Contributor
narfbg commented Nov 27, 2012

Even though the library is practically unmaintained and it's unlikely that somebody would create an e.g. mootols driver, it is supposed to have drivers.

@BillHeaton

Why?
.
On Nov 27, 2012, at 2:32 AM, Andrey Andreev wrote:

Even though the library is practically unmaintained and it's unlikely that somebody would create an e.g. mootols driver, it is supposed to have drivers.


Reply to this email directly or view it on GitHub.

@BillHeaton

It's not a case of the library is broken; It rather that the documentation is incorrect. The documentation changes that I've made, I've tested.

On Nov 26, 2012, at 2:03 PM, Andrey Andreev wrote:

@pickupman Go for it! I don't see many people enthusiastic about dealing with the Javascript library (even if it's broken), so any fix would be welcome.


Reply to this email directly or view it on GitHub.

@BillHeaton BillHeaton Fix Javascript Class/Driver Documentation (#17) (#2001) pass 4
Give a clue that the config can be passed when the library is loaded.
Shifted the CDN Comment to be closer to the example.
fe319a0
@narfbg
Contributor
narfbg commented Nov 27, 2012

Because it would otherwise be the Jquery library, not Javascript. :)

@narfbg narfbg and 1 other commented on an outdated diff Nov 27, 2012
user_guide_src/source/libraries/javascript.rst
-The Javascript class also accepts parameters, js_library_driver
-(string) default 'jquery' and autoload (bool) default TRUE. You may
-override the defaults if you wish by sending an associative array::
+ .. note:: The example above uses the `Google CDN
+ <http://developers.google.com/speed/libraries/devguide>`_
+ (Content Delivery Network) to host the script and there are some
+ advantages to doing so: decreased latency, increased parallelism,
+ and better caching. **However, you currently can't use the
@narfbg
narfbg Nov 27, 2012 Contributor

We don't need to have this notice in bold.

@BillHeaton
BillHeaton Nov 27, 2012

The code breaks in a bad way if you do it. Working on the fix. Can remove the notice when it's done.

@narfbg
narfbg Nov 28, 2012 Contributor

Since you have the "fix" already, this argument is pointless, but ... the code doesn't break, it just doesn't (or didn't) support protocol-less URIs. No example was given using such an URL, so that was fine without specifically mentioning and/or bolding it.

@narfbg narfbg and 1 other commented on an outdated diff Nov 27, 2012
user_guide_src/source/libraries/javascript.rst
- $this->load->library('javascript', array('js_library_driver' => 'scripto', 'autoload' => FALSE));
+You can set the configuration item in Applications/config/config.php, your own
@narfbg
narfbg Nov 27, 2012 Contributor

Applications -> application

@BillHeaton

@narfbg, re: javascript vs jquery. That was the question; However @pickman agrees with you, so good enough for me.

@BillHeaton

@narfbg: Sparks?

@BillHeaton BillHeaton Changes to support protocol-less urls.
Change the document to show the config example as the preferred Google
CDN url.  Change the code to accept protocol-less urls in external().
62541f1
@narfbg

Don't change this please.

Owner

You have asked me before to keep lines between 70-80 characters. Please be consistent.

I did, but it was on the docs, where (except for code blocks) it's been unofficially adopted as a hard limit.

On actual code it's more of a soft limit and unless it's hardly readable or doesn't fit your screen, then exceeding it is fine. Especially for lines like this - granted, it's lengthy, but still simple assignment with no extra expressions and/or concatenation present. Those are also just pointers and not strict rules, just use your common sense. :)

Another tip on making a pull request more easily acceptable is to not change what doesn't need to be changed. This particular line is of no importance here at all, it just makes the diff longer which means it's harder to go through and review.

Owner

I'm sure it's mentioned somewhere, but indeed - it's probably hard to find even if you're looking for it. And the docs are just now transitioned to Sphinx, so any old guides couldn't possibly cover that.
Usually just looking at the existing content is sufficient though - you won't see docs exceeding this limit, while you'll find plenty of code that does. Anything that might be unclear, I'm here to clarify. :)

Owner

Fair enough. However, We need to start writing that document.

@narfbg

The styleguide says that OR must be used instead of ||.
Additionally, we should probably just use regexp:

preg_match('/^(https?\:)\/\/', $external_file);

Also, this change (or should I say new feature) should be noted in the changelog.

Owner

It says that it's use is discouraged NOT that it must be used. I'm only adding enough code to support development at this point. Not willing to rewrite code until I've done the full analysis.

You asked me before not to put my changes in the change log. Please be consistent.

Okay, so I guess we'll have to update it to say it musn't be used. Still, if you compare it to how other comparison operators (and the INCORRECT vs. CORRECT examples), it is clear that you must use OR.

What is that full analysis that you're talking about? If you plan on introducing more changes to the code, then it would be better to first finish work on this PR and then submit a separate one for any other changes.

On the changelog - again, that was on the docs. Documentation updates don't belong in the changelog and I'm sure you can understand why. New functionalities and bug fixes (excluding documentation, if you consider incorrect examples to be bugs) must be listed in the changelog, otherwise we wouldn't have one. :)

Owner

The style guide lays out 2 basic rules on the operators:

  1. OR instead of ||
  2. && instead of AND

Anything else is just random thoughts on that topic, aimed at making it clear that it's about readability. And we're writing PHP after all. I didn't like these rules at first too, but we must obey them. Even if I merge this PR as is - somebody will immediately follow it with one replacing your || usage with OR.

On the rest - the title itself says that this is about fixing what's wrong with the JS docs. That should be the focus and scope of this pull request. Anything else is just delaying that goal from being accomplished and possibly introducing more questions to be asked and decisions to be made, which is not good. Not that further improvement isn't needed, it just has to be made step by step. Plus, it helps when you try to follow something on the commit history.

Owner

Forgive me but your misquoting the guide. However, I vote to drop it. I just made the change and It'll be going up as soon as I test.

Owner

It would be EllisLab that has to take that decision and from my experiences when talking to them about style guide changes, I don't expect them to agree. You are of course free to use whatever you like in your own code, it just can't stay as is in the CodeIgniter package.

On prettyprint - that's nice, but could it be integrated with GitHub? We are indeed spending a lot of time making contributors to fix any style inconsistencies in their patches. It's not a pleasant task, but somebody has to do it.

Owner

They added travis; prettyprint would probably be about the same amount of work.

@narfbg

If we'll be improving on readability, it might make more sense to move the dot on the next line.

Owner

ACK. Closing the line back up.

@narfbg

You still have an extra 's' at the end of applicationS.

Owner

ACK.

@narfbg

Using the Google CDN is great, but we're putting too much focus on that. Probably a better idea to put parenthesis around the last sentence.

Owner

No.

Because ... ?

Owner

Sorry, I was in a hurry to get to the Doctor. Goggle is prevalent example with Microsoft coming in a close 2nd. We could refer them to the jQuery download page but that seems awkward too. I think the easiest way is to just lose the last line. I think most people will just get it.

Agreed.

@narfbg
Contributor
narfbg commented Nov 28, 2012

@narfbg: Sparks?

What about Sparks?

@BillHeaton

What is Sparks?

On Nov 28, 2012, at 4:12 AM, Andrey Andreev wrote:

@narfbg: Sparks?

What about Sparks?


Reply to this email directly or view it on GitHub.

@narfbg
Contributor
narfbg commented Nov 28, 2012

A made-for-CodeIgniter package repository, you can see it at http://getsparks.org/.

It was at least planned to be integrated into the next major version of CI, but I'm not sure what the status of those plans is.

@pickupman pickupman added a commit to pickupman/CodeIgniter that referenced this pull request Nov 28, 2012
@pickupman pickupman Fix issue #17 and #2001 ad6be89
@pickupman
Contributor

@BillHeaton I merged your commits on the docs, and added a few more methods that were missing. I have updated the syntax for some of the points from Andrey. @narfbg I know there was talk of moving the javascript to a spark in issue #212 but seems that hasn't progressed either. Same talk about the cart class.
Either way, I have pushed all of these items to a branch on my fork ready for a pull request. The Javascript library has be fixed for being used properly as a driver as stated in the docs. I have also corrected a few bugs that existed in the external() method, as $this->_javascript_location was being set twice when adding additional scripts to be loaded.

I hope it wasn't an issue, but I did also add a __call() method to the parent driver to offer backward compatibility to the jQuery methods that are undocumented. (tablesorter, ui, and etc).

@pickupman
Contributor

Forgot to give and easier link to my javascript-driver branch.

The driver wasn't also properly supporting loading the jquery framework, and also allowing external plugins to be loaded as well. I have added an additional property to the driver to make this possible and more logical when autoloading the framework.

@BillHeaton BillHeaton Style Guide Changes
Style Guide Changes.  Kill the last line in the CDN note. Added the
changelog entry.
2e8fe7e
@BillHeaton

Love It! I just put the changes in to support protocol-less urls; now how do I incorporate your changes so that I can start testing? I am so new at github.

On Nov 28, 2012, at 1:38 PM, pickupman wrote:

Forgot to give and easier link to my javascript-driver branch.

The driver wasn't also properly supporting loading the jquery framework, and also allowing external plugins to be loaded as well. I have added an additional property to the driver to make this possible and more logical when autoloading the framework.


Reply to this email directly or view it on GitHub.

@pickupman
Contributor

@BillHeaton I am with you on the newbie part of git & github. Used Subversion then moved to mercurial. Git is getting easier. Here's what should work:

git remote add pickupman git://github.com/pickupman/CodeIgniter.git
git fetch pickupman
git checkout -b javascript-driver pickupman/javascript-driver

The first line will add access to my repo to get the changes
2nd line should get my repo changes
3rd line will create a new branch called javascript-driver in your remote so it does affect your other working branch, and then switch to the new javascript-driver branch.

You can switch back to your current changes by using
git checkout origin develop
#switch back
git checkout javascript-driver

@BillHeaton

Way Cool... I now have your changes which included my changes...

So I just do the normal commits/pushs and you'll see them too? Do we need to change the pull request for @narfbg or add a new one for the code changes? Lastly, Is this the right place to discuss suggested changes?

@BillHeaton

Opps... There were two changes to external()

line 689... should include strpos() for "https://" and "//" to allow them in _javascript_location as opposed to $external_file (passed as an argument) at line 685.

Was fixed in my branch; not in your branch... Should I just make the change since it's a small one?

BTW: I want to look a the entire external() chain, It can probably be simplified.

@pickupman
Contributor

Yeah it seems that the external() is a little messy. I think that it is trying to handle 2 different checks for how it should form the URL to the asset.

I can just pull your changes to update my branch.

@BillHeaton

It may be easier to do it manually... It's literally a one line change in external()...

On Dec 3, 2012, at 4:52 PM, pickupman wrote:

Yeah it seems that the external() is a little messy. I think that it is trying to handle 2 different checks for how it should form the URL to the asset.

I can just pull your changes to update my branch.


Reply to this email directly or view it on GitHub.

@narfbg narfbg closed this Oct 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment