pull in elm-html-test (and elm-html-in-elm and elm-html-query) #41
Conversation
- elm-html-test is not yet ready for 0.19, see elm-explorations/test#41
Fixed virtual DOM node decoding for normal nodes, text nodes, and lazy/thunk nodes
Update virtual-doc event handling code—event handlers are now VirtualDom.Handler instead of Json.Decode.Decoder
Fixed event handling when map, lazy, and keyed are used.
This changes Html.Test.Selector.style to take two strings instead of a list of String tuples (to parallel the change made in elm/html in Elm 0.19).
|
Just wanted to comment - great work on this @avh4! 😻 😻 😻 |
The event handlers are decoded in decodeFacts. The removed code could potentially be useful when using `Test.Html.Selector.attribute` with an event handler attribute, but the API doesn’t allow that, since `Test.Html.Selector.attribute` takes `Attribute Never` as its parameter.
|
@rtfeldman I think this is ready to merge if you want to look it over. cc @mgold |
There was a problem hiding this comment.
This was a joy to read. It's a huge PR, but it's so clean code it doesn't matter. No idea how much was written by whom (I feel like I found at least 2 distinct coding styles in this PR), but it doesn't really matter.
Wrote down a few comments, deal with them if you feel like it, I'm happy to merge this either way.
|
|
||
|
|
||
| // NOTE: this is duplicating constants also defined in Test.Internal.KernelConstants | ||
| // so if you make any changes here, be sure to synchronize them there! |
There was a problem hiding this comment.
I'm so happy you wrote this comment at the other site as well, pointing here! I fully expected there to be no such comment on the other side, because PR's at work would never have done that.
| else | ||
| isDescendant | ||
| (prependChildren current rest) | ||
| potentialDescendant |
There was a problem hiding this comment.
Would've expected this to be naively recursive. Nice one.
| -} | ||
| simulate : ( String, Value ) -> Query.Single msg -> Event msg | ||
| simulate = | ||
| Event |
There was a problem hiding this comment.
Could we get some named arguments here? It's not entirely clear to me what the arguments represent, mainly the pair.
| |> QueryInternal.failWithQuery showTrace | ||
| ("Event.expectEvent: Expected the msg \u{001B}[32m" | ||
| ++ Internal.toString msg | ||
| ++ "\u{001B}[39m from the event \u{001B}[31m" |
There was a problem hiding this comment.
Could we extract these escape codes into named values, so I can read what colors they represent?
| Expect.fail <| | ||
| String.join "\n" <| | ||
| List.concat | ||
| [ [ "Internal Error: failed to decode the virtual dom. Please report this at <https://github.com/elm-explorations/test/issues>." ] |
There was a problem hiding this comment.
This error message has a link to where the user can report the bug. Could we get that link in all the other "please report this bug" messages?
| = Find (List Selector) | ||
| | FindAll (List Selector) | ||
| | Children (List Selector) | ||
| -- First and Index are separate so we can report Query.first in error messages |
There was a problem hiding this comment.
Love this comment. The attention to detail is gold.
| queryName | ||
| ++ " always expects to find 1 element, but it found " | ||
| ++ String.fromInt resultCount | ||
| ++ " instead.\n\n\nHINT: If you actually expected " |
|
@drathier I believe all the changes you suggested are on code that was copied unchanged from other packages. I agree with them all, but think those would be best as a separate PR. Feel free to make one if you want! |
Co-Authored-By: rtfeldman <richard.t.feldman@gmail.com>
Co-Authored-By: rtfeldman <richard.t.feldman@gmail.com>
|
Thank you SO MUCH @avh4! You are amazing! |
|
I (most likely) won't do a PR to fix those comments. It's up for grabs for anyone else 🙌 |
will add it to my list for a future twitch stream :) |
|
Published as Thanks again @avh4! 🏆 |
No description provided.