Test files refactoring #220
Conversation
| @@ -15,7 +15,7 @@ | |||
| <link rel='stylesheet' href='assets/app.css'> | |||
| <link rel="stylesheet" href='assets/qunit.css'> | |||
|
|
|||
| <script src='assets/app.js'></script> | |||
| <script src='assets/tests.js'></script> | |||
There was a problem hiding this comment.
Ideally this should be
<script src='assets/app.js'></script>
<script src='assets/tests.js'></script>
|
me is soo jealous @stefanpenner @abuiles |
|
@stefanpenner just did the workaround we talked about and let as much to señor resolver. |
|
|
||
| testsIndexHTML = replace(testsIndexHTML, { | ||
| files: ['tests/index.html'], | ||
| patterns: [{ match: /\{\{ENV\}\}/g, replacement: getEnvJSON.bind(env)}] |
There was a problem hiding this comment.
I know this wasn't introduced by you, but i really dislike this. We should aim to clean this up sometime soon. (before the next release)
There was a problem hiding this comment.
The problem with rootURL is that it is needed before any JavaScript files can be loaded: It's used for setting the base tag and the base tag is used by the browser to resolve the relative URL of app.js.
|
i left 1 actionable comment, can you address and rebase? Then it should be good to go. |
| @@ -13,9 +13,10 @@ | |||
| </script> | |||
|
|
|||
| <link rel='stylesheet' href='assets/app.css'> | |||
| <link rel="stylesheet" href='assets/qunit.css'> | |||
| <link rel='stylesheet' href='assets/qunit.css'> | |||
There was a problem hiding this comment.
You gotta use double quotes in HTML.
There was a problem hiding this comment.
I thought single quote was fine, see https://github.com/stefanpenner/ember-cli/blob/master/blueprint/app/index.html#L4
There was a problem hiding this comment.
Oh man ^^' I didn't notice that there and I worked on that file only days ago. We NEED to change that there, too. Nobody uses single quotes in HTML. Single quotes are super unconventional. I see that you're really looking very closely :)
|
Updated. Just pending on the double quotes stuff, I thought single quotes was fine on html too. |
| @@ -4,6 +4,7 @@ | |||
| * Drops implementation files ([54df0288](https://github.com/twokul/ember-cli/commit/54df0288cd456aec782f0cbda269c603fe7be005)) | |||
| * Drop boilerplate tests ([c6f7475e](https://github.com/twokul/ember-cli/commit/c6f7475e0c8b3013b4af8ea5139aa25818aedeaf)) | |||
| * Use named-amd version of `ic-ajax` ([#225](https://github.com/stefanpenner/ember-cli/pull/225)) | |||
| * Separate `tests` and `app` code. Tests would be under 'assets/tests.js' (#220). | |||
There was a problem hiding this comment.
Grammar: Tests are now within 'assets/tests.js'
|
@MajorBreakfast updated again 👍 |
|
Rebase :) (only one commit, or rather one commit per logical step) And it would be nice if you could tackle app/index.html as well :) Thx! |
|
@MajorBreakfast rebased and did app/index.html too |
|
@MajorBreakfast done, actually the double quotes stuff might belong to a different PR, feel free to go ahead or I'll do it later. |
|
Okay :) I'll merge this now. |
First stab, there are still some things which I'm not quite happy yet, but we can work on that later.