-
Notifications
You must be signed in to change notification settings - Fork 28
TestContext.routeChange works with (some) relative URLs #3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Another, @brookeangel? |
| {-| This resolves a URL string (either an absolute or relative URL) against a base URL (given as a `Location`). | ||
| -} | ||
| resolve : Navigation.Location -> String -> Navigation.Location | ||
| resolve base url = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh, spooky - so it's okay that this allows people to get around the valid pathname constraint for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is actually how it should work (except for the unimplemented algorithm, of course).. this lets you call TestContext.routeChange in your test with a relative URL, and the relative URL you pass will be resolved against the current URL. So createWithNavigation still requires an absolute URL, but URLs given later will (hopefully) act the same as URLs in <a> tags that the user might click.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, got it! That seems right 👍
| -} | ||
| fail : String -> String -> TestContext msg model effect -> TestContext msg model effect | ||
| fail assertionName failureMessage (TestContext result) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that you added this function!
| } | ||
|
|
||
|
|
||
| createWithJsonStringFlags : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the options!!!
brookeangel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! I'm assuming more Location stuff is yet to come?
|
Location stuff is currently sufficient for what the NoRedInk repo needs. So I'm not planning to do more on that now, but can be added in the future as people need it. |
(and some unrelated additions of other functions)