-
-
Notifications
You must be signed in to change notification settings - Fork 116
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
Percent encode query #214
Percent encode query #214
Conversation
{-| Fixes issue with elm/url not properly escaping string | ||
-} | ||
queryString : String -> Query.Parser (Maybe String) | ||
queryString = | ||
Query.map (Maybe.andThen Url.percentDecode) << Query.string | ||
|
||
|
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.
Since Domen's PRs are still not merged we need to define fixed version ourselves.
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.
💯
[ Url.Parser.map Home Url.Parser.top | ||
, Url.Parser.map NotFound (Url.Parser.s "not-found") | ||
, Url.Parser.map Packages |
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 took a liberty and reformatted these parts a bit. This is all elm-format compatible.
{-| Fixes issue with elm/url not properly escaping string | ||
-} | ||
builderString : String -> String -> QueryParameter | ||
builderString name = | ||
Builder.string name << Url.percentEncode |
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.
Since Domen's PRs are still not merged we need to define fixed version ourselves.
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.
💯
src/Route.elm
Outdated
in | ||
"/" ++ String.join "/" path ++ "?" ++ String.join "&" (List.filterMap Basics.identity query) | ||
routeToString = | ||
uncurry Builder.absolute << routeToPieces |
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.
this is the only use of uncurry from basics-extra
createUrl toRoute channel query show from size sort = | ||
toRoute (Just channel) query show (Just from) (Just size) (Just <| toSortId sort) | ||
|> Route.routeToString |
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.
unification of URL building logic so we can keep fixes in single place.
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.
🥇
@@ -320,6 +313,7 @@ type Channel | |||
| Release_20_03 | |||
| Release_20_09 | |||
|
|||
|
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.
elm format.
@@ -102,7 +103,7 @@ update navKey msg model = | |||
let | |||
( newModel, newCmd ) = | |||
Search.update | |||
"options" | |||
Route.Options |
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.
❤️
a0398fe
to
171acbe
Compare
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've added the feedback. I'm closing the PR since @turboMaCk will push the branch to repo itself so we get nice previews.
import Url.Parser exposing ((<?>)) | ||
import Url.Parser.Query | ||
import Url.Parser.Query as Query |
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 to avoid using aliases and prefer to use absolute names.
I know it makes this bigger, but I find it much nicer when reading the code. Especially after some time, since I don't have to look at the header where the function is coming from.
If you don't have strong feelings about this I would like to revert this.
{-| Fixes issue with elm/url not properly escaping string | ||
-} | ||
builderString : String -> String -> QueryParameter | ||
builderString name = | ||
Builder.string name << Url.percentEncode |
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.
💯
createUrl toRoute channel query show from size sort = | ||
toRoute (Just channel) query show (Just from) (Just size) (Just <| toSortId sort) | ||
|> Route.routeToString |
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.
🥇
resolves #200
This PR has 2 commits:
createUrl
function in search withrouteToString
to ensure compatibility of urlsShowcase