Skip to content
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

checkboxesFieldList and multiSelectField and any other multiple field lose information on error #1449

Open
ricksanchez opened this issue Oct 21, 2017 · 2 comments

Comments

@ricksanchez
Copy link

The Field has some strange design:
the fieldParse and the fieldView has something that looks like contradiction.
See the types:
fieldParse :: [Text] -> [FileInfo] -> m (Either (SomeMessage (HandlerSite m)) (Maybe a))
fieldView ::FieldViewFunc m a

FieldViewFunc sadly doesn't follow the design of fieldParse.

fieldParse takes [Text] (n inputs) but fieldView takes Left Text a (only one). This is a big limitation for implementing dynamic forms that has "add row" option with a common submit.

This design affects the checkboxesFieldList and multiSelectField (sabotage only one value, and the whole widget will be blank).

Would it be wise to change the type of FieldViewFunc to

type FieldViewFunc m a
    = Text -- ^ ID
   -> Text -- ^ Name
   -> [(Text, Text)] -- ^ Attributes
   -> [Either Text a] -- ^ A list of Either (invalid text) or (legitimate result)
   -> Bool -- ^ Required?
   -> WidgetT (HandlerSite m) IO ()

?

@ricksanchez ricksanchez changed the title checkboxesFieldList and multiSelectField and any other multiple field loses information on error checkboxesFieldList and multiSelectField and any other multiple field lose information on error Oct 21, 2017
@snoyberg
Copy link
Member

It's probably a worthwhile change, and may not have a massive backwards compatibility hit (since I'm guessing most people are using built-in fields). I haven't thought through the impacts of this yet, and it may have negative results that prevent it from being possible.

The best way to move forward would likely be a PR making the change, though anyone who works on it needs to be aware that there's a real possibility that the PR wouldn't be accepted. Just don't want to set any false expectations.

@nmk
Copy link
Contributor

nmk commented Jul 26, 2018

This is an actual bug though. As @ricksanchez points out, a checkboxesField (and I assume multiSelectField as well) which fails validation loses the checked state of the checkboxes. Here is a demonstration: https://gist.github.com/adbd511ecd1144ed64dd75f9bcccbd4e

I ran into this while looking around the code for #1541. Fixing the problem there is easy, as a select field only has one submitted value, a checkboxesField has multiple, yet in the Left case in https://github.com/yesodweb/yesod/blob/master/yesod-form/Yesod/Form/Fields.hs#L501 only the first submitted value is to be found, there is no way to salvage the other ones.

Does anyone have a pointer on where to start looking in order to fix this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants