Remaining TodoMVC test failures

[passy]

I think the first confusingly titled "should allow me to add todo items" failure refers to the localStorage persistence.

Saving on blur is implemented slightly differently. You're currently reverting the edit on blur and escape is ignored. I tried to change it like this, but I must be missing something because the behavior is not what I expected. I think because escape also blurs the entry, it's not as easy as I thought:

diff --git a/Task.elm b/Task.elm
index 5e65045..6a0bcab 100644
--- a/Task.elm
+++ b/Task.elm
@@ -113,18 +113,25 @@ view channel task =
           , name "title"
           , id ("todo-" ++ toString task.id)
           , on "input" targetValue (\desc -> LC.send channel (task.id, Edit desc))
-          , onBlur (LC.send channel (task.id, Cancel))
+          , onEscape (LC.send channel (task.id, Cancel))
+          , onBlur (LC.send channel (task.id, Commit))
           , onEnter (LC.send channel (task.id, Commit))
           ]
           []
       ]

+onEscape : Signal.Message -> Attribute
+onEscape message =
+    on "keydown"
+      (Json.customDecoder keyCode (isKey 27))
+      (always message)
+
 onEnter : Signal.Message -> Attribute
 onEnter message =
     on "keydown"
-      (Json.customDecoder keyCode is13)
+      (Json.customDecoder keyCode (isKey 13))
       (always message)

-is13 : Int -> Result String ()
-is13 code =
-  if code == 13 then Ok () else Err "not the right key code"
+isKey : Int -> Int -> Result String ()
+isKey code input =
+  if input == code then Ok () else Err "not the right key code"

The "back button" means that the routing should be observed and using back/forwards should change the All/Active/Completed filter in the app.

[luigy]

It was fun to try to fix these tests to finally play with and get a feeling for Elm https://github.com/luigy/elm-todomvc/commit/c85fcf42a4421d4e69869bb556ff27abe9cf80dd

[passy]

@luigy That's looking great! :)

@evancz Any chance this could get pulled in?

[evancz]

@luigy, is it okay for me to merge these into my branch? My stuff is all BSD3 and I try to be very careful about this.

So the last remaining thing is local storage? I vaguely recall that not being needed, but I guess I was confused. Once we hear back from @luigy, I'll bring that over from the master branch. At that point, it'll probably be all set.

[luigy]

@evancz it needs to be cleaned up, though:

• logic for visibility is duplicated. ChangeVisibility action can be removed or the router could "send" the action
• the router sends the hash formatted for taskList case-expression, but it doesn't check wether it is valid "visibility". Maybe the True case could be relaxed or this logic be handled in the router
• I was thinking in renaming router to 'location' and just pass the raw location and do all the formatting in Elm as well

Since, "TodoMVC" is usually used as an example to follow I wasn't sure how to go about code style or naming in Elm so I was hoping someone familiar with it to critique :smile:

If you would like to tell me how to proceed I can submit a PR or feel free to merge with the proper changes :)

[passy]

@evancz Yup, localStorage is still needed. We have an experimental (and thus optional) API, but localStorage is mandatory in terms of the spec at this point.

[evancz]

Okay, I think all of the things mentioned here should be covered now. @luigy, I just left the URL parsing stuff in JS for now, though I could imagine a pattern match that only does stuff on valid strings.

[passy]

29 passing (1m)

Fantastic! \o/

There are some stylistic changed we have to make to match the TodoMVC repo. It's up to you whether we want to do this in this repo or whenever we're syncing the versions. It's only about white spaces and tabs. What do you think?

[evancz]

What are the rules of that change? Tabs are not permitted in Elm, and though it's possible to do things "all 4 space indent" it would not look very typical.

[passy]

Tabs are not permitted in Elm

Okay, that makes things easier. :) Other than that the only thing we enforce through JSCS/JSHint is a space after function, ie. function () { …. Otherwise it should be perfectly compatible.

[evancz]

Do you mean in the snippet of JS in index.html? I'm happy to make those changes on this repo.

[passy]

@evancz Yup, that's what I mean. Would you like to submit the PR afterwards?

[evancz]

I'm not sure exactly what is needed right now. Would you mind doing a PR for the JS changes? I'm not sure if function foo () also needs a space, and I think it's probably easier to just do it than explain to me exactly what needs to happen.

I'm not sure what the protocol is for doing PRs to the todomvc repo, so if I can just get cc'd on it, that'd be ideal.

Is that okay?

[passy]

@evancz Sounds good. I think I still have a pending one, I can just update that one and cc you on it. :)

[passy]

The discussion is now in https://github.com/tastejs/todomvc/pull/1041