Issue w/ head.load #522
Issue w/ head.load #522
Conversation
|
this PR tries to fix #281 // cc @matteofigus |
| done(); | ||
| }); | ||
| }); | ||
| module.exports = function(grunt) { |
There was a problem hiding this comment.
this has been reformatted because of prettier
src/components/oc-client/src/l.js
Outdated
| } | ||
| } | ||
| //export ljs (as `head`) | ||
| window.head = loader; |
There was a problem hiding this comment.
this is the only workaround applied 😅
There was a problem hiding this comment.
Well this is temporary right? If we would want to go for it, we can change the library to make it use ljs instead and it should work I guess
There was a problem hiding this comment.
do you want me to use ljs instead of head as the name of the loader? sure I'll try to do that. I will also test CSS loading.
There was a problem hiding this comment.
C'est formidable!
I think that this library does the job well, and despite not being super active, it makes all the tests pass, so I would say let's go for it.
I see that you did a good job on setting up a manual test too. The only think we may want to test (which I think is not covered by the karma tests) is css loading with oc.require. This library should support it according to the docs, but I just to ensure that we can keep all retro-compatible ;)
|
@matteofigus I updated this PR that now uses a global |
| } | ||
| } | ||
| //export ljs | ||
| window.ljs = loader; |
There was a problem hiding this comment.
as per the lib now the global name is ljs
|
Sounds good. Let's remember to update this file too https://github.com/opentable/oc/blob/master/src/components/oc-client/LICENSES |
| @@ -27,7 +27,7 @@ SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. | |||
| OC wouldn't exist without some fantastic Open-Source libraries: | |||
|
|
|||
| * jQuery | |||
| * head.js | |||
| * l.js | |||
There was a problem hiding this comment.
@matteofigus let me know if this is good for you
|
all 💚 over to you @matteofigus // cc @NapoleanReigns |
|
Fantastic. I decided to go live with a new minor (0.38.0) because despite this should be a non breaking change, there was a non explicit contract on exposing |
|
I wanted to wait a couple of weeks (just to make sure that there was no production issue) but now I'd like to take some time and thank @malko for his // cc @matteofigus @nickbalestra |
|
@mattiaerre thank for your mention and use of ljs. Glad it can help you. As said earlier in the comment the repository is not really active but i want to assure you that this is not a dead project, I still accept PR and take a look to it on a regular basis. I just don't have more need for it at this time but as said PR are welcome as long as it don't forget the purpose of ljs, stay a micro library. |
Description
the following gif shows that when replacing
head.loadw/lit is possible to wait until the asset has been loaded before firing a callback function; this PR includes also the 2 html files used in order to record the video.the first part shows the behavior w/
head.loadsimulating a Regular 2G network w/ Chrome dev tool; the second part shows the behavior w/lsimulating the same network speed.The test is fetching a
.cssand a.jsfile.the source code of the
llibraryChanges
add the
llibrary (and update .eslintignore)update
build.jsso that it is includinglinsideoc-client.min.jswhengrunt builduse
linstead ofhead.loadinside karma-common (grunt karma:local still works)