Add Cordova build target #3
Conversation
michielbdejong
force-pushed
the
cordova-target
branch
from
f2cdc02
to
459665a
Compare
michielbdejong
changed the title
|
r? @gmarty Thanks for looking into the ES5 issue, that worked! :) |
gmarty
force-pushed
the
master
branch
2 times, most recently
from
d5a0448
to
a74105f
Compare
michielbdejong
force-pushed
the
cordova-target
branch
from
459665a
to
98f2b79
Compare
| @@ -16,6 +16,7 @@ | |||
| <link rel="icon" sizes="512x512" href="img/icons/512.png"> | |||
| <meta name="application-name" content="Foxbox Client"> | |||
| <title>Foxbox Client</title> | |||
| <script type="text/javascript" src="cordova.js"></script> | |||
There was a problem hiding this comment.
This is needed for the Cordova build; I added an empty app/cordova.js file to the repo so that it won't error when using gulp to serve the app on http://localhost:8000/. We should do this in a fancier way eventually, but I think for now it does the trick. :)
There was a problem hiding this comment.
Can you remove the type="text/javascript" attribute and add a defer one, for the sake of consistency?
|
This PR looks good to me, but I think we should wait until the es5 issue is fixed in the MVC repo then we can just update the dependencies. This issue is being tracked at fxos-eng/sanitizer#3. Let's see how it goes. |
|
OK! Will merge it without the |
| @@ -0,0 +1 @@ | |||
| <!-- to be generated during Cordova build --> | |||
There was a problem hiding this comment.
Can you make it a regular comment? It seems that HTML style comments are not supported by Microsoft browsers.
There was a problem hiding this comment.
Ah of course, this is a .js file, wrong comment style.
gmarty
force-pushed
the
master
branch
6 times, most recently
from
09bf09c
to
b11a123
Compare
| cordova platform add android | ||
| cordova build android | ||
| cordova emulate android | ||
| ``` |
There was a problem hiding this comment.
Do you think you can make this a gulp task in the gulpfile.js instead?
The command to build would just have to be gulp cordova.
There was a problem hiding this comment.
ok, will have a look at that!
There was a problem hiding this comment.
Done. I couldn't merge gulp cordova-setup and gulp cordova-android into one task because the second has to run in a folder (dist/cordova), which the first one creates. So I left it as two tasks, hope that's ok.
michielbdejong
force-pushed
the
cordova-target
branch
2 times, most recently
from
9c7a6c7
to
cef51be
Compare
Can we just merge it now and then iterate later? |
| cordovaHTTP && cordovaHTTP.acceptHss) { | ||
| this.supported = true; | ||
| } else { | ||
| this.supported = true; |
There was a problem hiding this comment.
Shouldn't this line read?
this.supported = false;There was a problem hiding this comment.
Thanks! Fixed.
michielbdejong
force-pushed
the
cordova-target
branch
2 times, most recently
from
026c422
to
30df6bd
Compare
|
@gmarty addressed your latest review comments. |
Looks like
gulpis building ES6 - should find out how to generate ES5 code.