-
Notifications
You must be signed in to change notification settings - Fork 374
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
Implemenatation of chooseRadio function along with tests #1840
Conversation
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 an excellent addition the the library, though I think we ought to tidy up some minor details first.
yesod-test/Yesod/Test.hs
Outdated
-- > <label for="color-red">Red</label> | ||
-- > <input id="color-red" name="color" value="Red" /> |
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 typical radio input markup should look like this:
<input id="color-red" name="color" value="red">
<label for="color-red">Red</label>
i.e., the input should come before the label. Also, self-closing tags don't need a trailing slash.
yesod-test/Yesod/Test.hs
Outdated
-- @since 1.6.17 | ||
chooseRadio :: T.Text -- ^ The text contained within the @\<label>@. | ||
-> RequestBuilder site () | ||
chooseRadio v = do |
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 v
could be a better name. Perhaps labelText
?
yesod-test/Yesod/Test.hs
Outdated
mres <- fmap rbdResponse getSIO | ||
res <- | ||
case mres of | ||
Nothing -> failure "chooseRadio: No response available" | ||
Just res -> return res |
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 you're only using mres
in one place, you may as well inline it.
mres <- fmap rbdResponse getSIO | |
res <- | |
case mres of | |
Nothing -> failure "chooseRadio: No response available" | |
Just res -> return res | |
res <- case rbdResponse <$> getSIO of | |
Nothing -> failure "chooseRadio: No response available" | |
Just res -> pure res |
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'm pretty sure this can't be inlined unless using LamdbdaCase >>= \case
? Otherwise we need >>= (\v -> case v of ...)
but I think that is less readable but happy to do that if preferred.
yesod-test/Yesod/Test.hs
Outdated
case mres of | ||
Nothing -> failure "chooseRadio: No response available" | ||
Just res -> return res | ||
let body = simpleBody res |
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.
Similar deal here — this binding is only used once, so it may as well be inlined below.
yesod-test/Yesod/Test.hs
Outdated
Just res -> return res | ||
let body = simpleBody res | ||
|
||
let name = genericNameFromHTML (==) (v) body |
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.
Unless my eyes (and brain) are failing me, I think the parentheses are redundant here.
let name = genericNameFromHTML (==) (v) body | |
let name = genericNameFromHTML (==) labelText (simpleBody res) |
yesod-test/Yesod/Test.hs
Outdated
Right name' -> addPostParam name' $ v | ||
Left e -> failure $ e |
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.
The function application operators are redundant in both cases. Also, let's try to use slightly more descriptive names 🙂
Right name' -> addPostParam name' $ v | |
Left e -> failure $ e | |
Right name' -> addPostParam name' labelText | |
Left err -> failure err |
Thanks, will try sort this all out later today. |
…opriate example html.
Closing this in favour of #1842. |
Before submitting your PR, check that you've:
@since
declarations to the Haddocks for new, public APIsAfter submitting your PR:
I've been asked to upstream this work, so here it is! It's basically a helper function to make it easier to set radio form inputs.