-
Notifications
You must be signed in to change notification settings - Fork 17
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
Add validation functions for textArea and dropdown #82
base: develop
Are you sure you want to change the base?
Conversation
{ _validationConfig_feedback = const blank | ||
, _validationConfig_errorText = id | ||
, _validationConfig_validation = const $ toDynValidation $ pure $ Left "Validation not configured" | ||
, _validationConfig_validationM = Nothing | ||
, _validationConfig_initialAttributes = mempty | ||
, _validationConfig_validAttributes = mempty | ||
, _validationConfig_invalidAttributes = mempty | ||
, _validationConfig_initialValue = "" |
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.
Maybe require Monoid v here and add a second mkBaseValidationConfig
or something without that constraint that takes an argument.
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.
Okay. Does it make sense to deprecate defValidationConfig
in the process? Because I wonder about the value of having two functions (with non-similar names to boot).
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.
mkValidationConfig
seems like a better name since "base" is not in the type's name. You can't deprecate the argument-taking one because v
may not be monoidal. (Unless you expect people to construct from the record directly...)
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.
You can't deprecate the argument-taking one
Yea, but I meant: deprecate the other one (one that has the Monoid constraint).
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.
Isn't that the one that @eskimor is proposing you add? Maybe I missed something.
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 not having to provide an argument for the common case is useful and would make this change less breaking (maybe not breaking at all). So I don't think we need to deprecate anything.
{ _validationDropdown_input :: Dropdown t (Maybe a) | ||
, _validationDropdown_value :: DynValidation t e a | ||
} | ||
|
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.
Why do we need validation for a dropdown? Shouldn't we just only offer options that are valid?
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.
@eskimor One of the items in the dropbox can be "Select an item" (which would be the default selection). If the user has not in fact selected one of the other items, we would want to display a validation error.
The "Select an item" thing would be Nothing
, and the other items Just a
. So we would just use the validateJust
function here.
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.
Makes sense. Thanks!
a5c5482
to
c82d0e6
Compare
(cherry picked from commit a5c5482)
c82d0e6
to
5b334cd
Compare
This PR now targets the |
To compensate for the fact that Dropdown has no HasDomEvent instance (cherry picked from commit fbe9e2b)
The current
validationInput
only works withinputElement
. This PR creates two similar functions to work withtextArea
anddropdown
.Unlike the other two input types, dropdown input's values can be of arbitrary type,
v
, instead of necessarily beingText
. To accommodate thisValidationConfig
type now takes an extra type parameter,v
, that specifies this type. ForvalidationInput
andvalidationTextArea
this isText
, but forvalidationDropdown
it is the user that will provide this type.