Olives: update to olives and emily 3.x, remove dependency on requirejs, Fixes #1029 #1242

podefr · 2015-03-31T02:19:05Z

Hi,

This PR should fix #1029 (focus on load). The issue was fixed by updating olives and emily to their 3.0.x version.

The changes in this PR are:

Update to emily 3.0.3
Update to olives 3.0.5
Remove dependency on requirejs
Use node's require to replace requirejs
Use browserify to bundle the application
Update HTML to only load the bundle
Add a "build" and "watch" scripts to package.json to build the app
Use lowercase names for the UI components as they were never actually newed up.

This PR doesn't add the routing feature, this will come in a future PR as I'm working on it.

Thanks,
Olivier

arthurvr · 2015-03-31T04:46:42Z

Thanks! I'll review this shortly.

arthurvr · 2015-03-31T15:22:14Z

examples/olives/package.json

+  },
+  "scripts": {
+      "build": "browserify ./js/app.js -o olives-todo.js",
+      "watch": "watchify ./js/app.js -o olives-todo.js"


package.json files use 2 spaces indentation.

arthurvr · 2015-03-31T15:28:05Z

This PR should fix #1029 (focus on load). The issue was fixed by updating olives and emily to their 3.0.x version.

Works for me in firefox, but not in Chrome or Safari. Could you double-check it?

arthurvr · 2015-03-31T15:29:11Z

This PR doesn't add the routing feature, this will come in a future PR as I'm working on it.

Thanks, awesome!

podefr · 2015-03-31T15:30:09Z

Thanks for your feedback, I'll update this later on today and will push a new commit.

podefr · 2015-04-01T02:43:26Z

I converted the spaces to tabs and added the newlines at the bottom of each JS file, although they don't seem to appear in github.
I double-checked in safari 8.0.4/chrome 43/firefox 31, and I do get the autofocus on the input field on load. Is anybody else seeing the same issue?

podefr · 2015-04-01T02:44:12Z

oh and btw, let me know if you want me to squash the commits before merging

arthurvr · 2015-04-01T12:49:43Z

LGTM at the moment. No idea what was going on with the autofocus as atm it works fine for me as well :/ All tests pass for olives except the routing. Awesome work @podefr! If nobody objects I'm gonna land this one.

arthurvr · 2015-04-01T12:54:13Z

examples/olives/package.json

+    "olives": "^3.0.5",
+    "todomvc-app-css": "^1.0.1",
+    "todomvc-common": "^1.0.1"
+  },


Oh, one little thing, Browserify and watchify will need to be in the dev deps.

good point, I'll fix it and push a new commit later on today

@podefr Thanks

podefr · 2015-04-02T03:47:24Z

I've added browserify and watchify to the package.json and squashed all the commits. Thanks for taking the time to review this @arthurvr!

arthurvr · 2015-04-02T07:45:59Z

Great!

samccone · 2015-04-02T13:19:38Z

examples/olives/js/lib/Tools.js

@@ -1,4 +1,3 @@
-/*global define*/
 (function () {


the iffy wrapper seems odd now that we are using commonjs right?

samccone · 2015-04-02T13:20:47Z

I am not sure we want to delete

examples/olives/node_modules/todomvc-app-css/index.css

podefr · 2015-04-03T20:14:13Z

@samccone, index.css hasn't been deleted, don't know why github is confused. It has been modified though, probably when updating the dependencies.

Here's what the file looks like as of this commit:

https://github.com/tastejs/todomvc/tree/92fdf4fbd67132df26ee468d46304bcbe9cbad88/examples/olives/node_modules/todomvc-app-css

podefr · 2015-04-08T03:34:58Z

@arthurvr @samccone I believe I've answered all your comments, let me know if there's anything else I can do so you can close this, thanks.

arthurvr · 2015-04-08T06:13:39Z

examples/olives/js/lib/Tools.js

+        } else {
+            this.classList.remove(className);
+        }
+    }


For this file same thing as #1242 (comment)

And it looks like all the other files as well. package.json is the only one that shouldn't use tabs.

podefr · 2015-04-09T01:50:24Z

@arthurvr done!

samccone · 2015-04-09T13:30:41Z

hi @podefr looks like you have a failing spec

 TodoMVC - olives
    When page is initially opened
      ✓ should focus on the todo input field
    No Todos
      ✓ should hide #main and #footer (74ms)
    New Todo
      ✓ should allow me to add todo items (381ms)
      ✓ should clear text input field when an item is added (175ms)
      ✓ should append new items to the bottom of the list (468ms)
      ✓ should trim text input (213ms)
      ✓ should show #main and #footer when items added (221ms)
    Mark all as completed
      ✓ should allow me to mark all items as completed (494ms)
      ✓ should allow me to clear the completion state of all items (556ms)
      ✓ complete all checkbox should update state when items are completed / cleared (585ms)
    Item
      ✓ should allow me to mark items as complete (480ms)
      ✓ should allow me to un-mark items as complete (441ms)
      ✓ should allow me to edit an item (771ms)
      ✓ should show the remove button on hover
    Editing
      ✓ should hide other controls when editing (443ms)
      ✓ should save edits on enter (666ms)
      ✓ should save edits on blur (847ms)
      ✓ should trim entered text (818ms)
      ✓ should remove the item if an empty text string was entered (634ms)
      ✓ should cancel edits on escape (735ms)
      1) "after each" hook

  20 passing (45s)
  1 failing

  1) TodoMVC - olives "after each" hook:
     Uncaught AssertionError: 3 items expected in the todo list, 2 items observed
      at /Users/sam/Desktop/repos/todomvc/browser-tests/testOperations.js:126:12
      at _fulfilled (/Users/sam/Desktop/repos/todomvc/browser-tests/node_modules/q/q.js:794:54)
      at self.promiseDispatch.done (/Users/sam/Desktop/repos/todomvc/browser-tests/node_modules/q/q.js:823:30)
      at Promise.promise.promiseDispatch (/Users/sam/Desktop/repos/todomvc/browser-tests/node_modules/q/q.js:756:13)
      at /Users/sam/Desktop/repos/todomvc/browser-tests/node_modules/q/q.js:564:44
      at flush (/Users/sam/Desktop/repos/todomvc/browser-tests/node_modules/q/q.js:110:17)
      at process._tickCallback (node.js:419:13)

podefr · 2015-04-13T03:51:20Z

@samccone I pulled down the latest master and ran:

npm run test -- --framework=olives

but I'm not seeing any failing test apart from routing. Can you double-check?

   TodoMVC - olives
    When page is initially opened
      ✓ should focus on the todo input field
    No Todos
      ✓ should hide #main and #footer (79ms)
    New Todo
      ✓ should allow me to add todo items (313ms)
      ✓ should clear text input field when an item is added (145ms)
      ✓ should append new items to the bottom of the list (336ms)
      ✓ should trim text input (149ms)
      ✓ should show #main and #footer when items added (185ms)
    Mark all as completed
      ✓ should allow me to mark all items as completed (366ms)
      ✓ should allow me to clear the completion state of all items (397ms)
   TodoMVC - olives
    When page is initially opened
      ✓ should focus on the todo input field
    No Todos
      ✓ should hide #main and #footer (73ms)
    New Todo
      ✓ should allow me to add todo items (303ms)
      ✓ should clear text input field when an item is added (143ms)
      ✓ should append new items to the bottom of the list (347ms)
      ✓ should trim text input (154ms)
      ✓ should show #main and #footer when items added (185ms)
    Mark all as completed
      ✓ should allow me to mark all items as completed (369ms)
      ✓ should allow me to clear the completion state of all items (381ms)
      ✓ complete all checkbox should update state when items are completed / cleared (424ms)
    Item
      ✓ should allow me to mark items as complete (375ms)
      ✓ should allow me to un-mark items as complete (326ms)
      ✓ should allow me to edit an item (524ms)
      ✓ should show the remove button on hover
    Editing
      ✓ should hide other controls when editing (318ms)
      ✓ should save edits on enter (521ms)
      ✓ should save edits on blur (612ms)
      ✓ should trim entered text (533ms)
      ✓ should remove the item if an empty text string was entered (471ms)
      ✓ should cancel edits on escape (449ms)
    Counter
      ✓ should display the current number of todo items (232ms)
    Clear completed button
      ✓ should display the number of completed items (375ms)
      ✓ should remove completed items when clicked (403ms)
      ✓ should be hidden when there are no items that are completed (369ms)
    Routing
      1) should allow me to display active items
      2) should respect the back button
      3) should allow me to display completed items
      4) should allow me to display all items
      5) should highlight the currently applied filter


  24 passing (44s)
  5 failing

podefr · 2015-04-19T17:07:38Z

@samccone @arthurvr seems like the tests are passing for me (expect for routing), can you double check and maybe close this PR that's been open for 20 days? I'll add routing as part of another PR.

Thanks,
Olivier

arthurvr · 2015-04-19T19:18:58Z

@podefr Indentation is still wrong. Otherwise I'm fine with landing this as olives doesn't pass the specs in master too.

podefr · 2015-04-19T19:36:20Z

thanks @arthurvr, fixing the indent now. I had never saw the comment above about using tabs instead of spaces :/

podefr · 2015-04-19T19:42:46Z

@arthurvr done

arthurvr · 2015-04-24T19:59:46Z

@samccone I can actually confirm the failing test, but the spec which is failing is due to #789, which is another issue and thus not topic for this PR.

arthurvr · 2015-04-24T20:01:26Z

examples/olives/package.json

+  "scripts": {
+    "build": "browserify ./js/app.js -o olives-todo.js",
+    "watch": "watchify ./js/app.js -o olives-todo.js"
+  },


Mind adding a note about these scripts in the examples/olives/readme.md? Thanks!

arthurvr · 2015-04-24T20:02:13Z

one last comment, for the rest this looks good.

arthurvr · 2015-04-24T20:02:20Z

Oh, and, thanks for your hard work!

samccone · 2015-04-24T20:02:56Z

👏

Fixes tastejs#1029

podefr · 2015-04-26T20:41:09Z

@arthurvr @samccone I've updated the README with the notes about the two scripts. When this is merged in, I'll add the router

arthurvr · 2015-04-27T17:09:58Z

examples/olives/readme.md

+npm install
+```
+
+


remove these lines :)

arthurvr · 2015-04-27T17:12:33Z

Instead of nitpicking more I'll land this this and do some tweaks myself! Once again, awesome work!

arthurvr · 2015-04-27T17:26:18Z

When this is merged in, I'll add the router

That would be super awesome!

* Update to the latest version of the Olives framework. * Update to the latest version of Emily * Remove requirejs in favor of browserify - also implemented npm scripts and updated the readme As a result of updating dependencies, this fixes the issue in tastejs#1029. Ref tastejs#1029 Close tastejs#1242

podefr changed the title ~~Olives: update to olives and emily 3.x, remove dependency on requirejs~~ Olives: update to olives and emily 3.x, remove dependency on requirejs, Fixes #1029 Mar 31, 2015

podefr mentioned this pull request Mar 31, 2015

Focus on load #1029

Open

7 tasks

podefr force-pushed the update-olives-3 branch from 7951647 to 8d18bb5 Compare March 31, 2015 02:44

arthurvr reviewed Mar 31, 2015
View reviewed changes

arthurvr reviewed Apr 1, 2015
View reviewed changes

podefr force-pushed the update-olives-3 branch from d97ed60 to 05e6a07 Compare April 2, 2015 03:44

samccone reviewed Apr 2, 2015
View reviewed changes

podefr force-pushed the update-olives-3 branch 2 times, most recently from af586c1 to 92fdf4f Compare April 3, 2015 20:00

arthurvr reviewed Apr 8, 2015
View reviewed changes

podefr force-pushed the update-olives-3 branch from 92fdf4f to 94af548 Compare April 9, 2015 01:47

podefr force-pushed the update-olives-3 branch from 94af548 to d500706 Compare April 19, 2015 19:41

arthurvr reviewed Apr 24, 2015
View reviewed changes

Olives: update to olives and emily 3.x, remove dependency on requirejs.

082b6af

Fixes tastejs#1029

podefr force-pushed the update-olives-3 branch from d500706 to 082b6af Compare April 26, 2015 20:38

arthurvr reviewed Apr 27, 2015
View reviewed changes

examples/olives/readme.md

npm install

```

Copy link

Member

arthurvr Apr 27, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove these lines :)

arthurvr closed this in 5a3c81c Apr 27, 2015

Olives: update to olives and emily 3.x, remove dependency on requirejs, Fixes #1029 #1242

Olives: update to olives and emily 3.x, remove dependency on requirejs, Fixes #1029 #1242

Conversation

podefr commented Mar 31, 2015

arthurvr commented Mar 31, 2015

Choose a reason for hiding this comment

arthurvr commented Mar 31, 2015

arthurvr commented Mar 31, 2015

podefr commented Mar 31, 2015

podefr commented Apr 1, 2015

podefr commented Apr 1, 2015

arthurvr commented Apr 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

podefr commented Apr 2, 2015

arthurvr commented Apr 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samccone commented Apr 2, 2015

podefr commented Apr 3, 2015

podefr commented Apr 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

podefr commented Apr 9, 2015

samccone commented Apr 9, 2015

podefr commented Apr 13, 2015

podefr commented Apr 19, 2015

arthurvr commented Apr 19, 2015

podefr commented Apr 19, 2015

podefr commented Apr 19, 2015

arthurvr commented Apr 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arthurvr commented Apr 24, 2015

arthurvr commented Apr 24, 2015

samccone commented Apr 24, 2015

podefr commented Apr 26, 2015

Choose a reason for hiding this comment

arthurvr commented Apr 27, 2015

arthurvr commented Apr 27, 2015