Vanguyen/dev - #7
Conversation
n8mellis
left a comment
n8mellis
left a comment
There was a problem hiding this comment.
Looking good! A few comments and questions included inline.
| value={this.state.value} | ||
| onChange={this._handleChange.bind(this)} > | ||
| {emptyPlaceholder} | ||
| {options} |
There was a problem hiding this comment.
I like what you did here, but we should probably use either the options data or the children, not both. It would be unclear what the behavior should be for supporting both. My first thought is that having children should trump supplying options data. Thoughts?
| * This component returns a <form> with legend. | ||
| * | ||
| * Required props: | ||
| * - legend: string. sets the form legend |
There was a problem hiding this comment.
Can the legend ever be more complex than a simple string?
There was a problem hiding this comment.
I'm not sure...there could be situations where it could be a variable that is customized? Like a Name? Or I am not understanding the question...
| return ( | ||
| <div> | ||
| <form className="usa-form"> | ||
| <fieldset className="usa-fieldset-inputs usa-sans"> |
There was a problem hiding this comment.
I'm not sure we always want a top-level fieldset. Fieldsets should be used to group related form elements together so having a form-global one doesn't make a lot of sense. Thoughts?
| <Form | ||
| legend="Name" | ||
| > | ||
| <TextInput |
There was a problem hiding this comment.
Per my comment earlier, I would recommend that we put the fieldset and legend inside this component, not Form. I'm also thinking that we might want to just return a fieldset as the top level rather than a form so that we can mix these fields in to a pre-existing form. In that case we'd probably want to name this NameFields or something like that. Thoughts? @mstanaland thoughts?
| * - errorMessage: string. If present triggers the error state and displays the error message | ||
| * - isValid: bool. If true, sets isValid state - i.e. green border | ||
| * - allowedChars: Validator object. If present, user can only type characters that match the given test | ||
| * - inputClass: string. If present, adds an custom class to field, for UI specific targeting |
There was a problem hiding this comment.
Can we re-use the className prop that is already supported? If we don't need to make people learn a new prop name, then we shouldn't.
… in general form components
|
@n8mellis Required Fields: I'm not sure that there should be inconsistency between labeling a required field for the Name form and the Address form, see documentation under Address form: |
…ent to handle other props, IN PROGRESS
|
|
||
| PrimaryButton.propTypes = { | ||
| disabled: React.PropTypes.bool, | ||
| onClick: React.PropTypes.function, |
There was a problem hiding this comment.
@n8mellis this was giving an error in console:
Failed prop type: PrimaryButton: prop type onClick is invalid; it must be a function, usually from React.PropTypes. in PrimaryButton (created by SignInForm)
There was a problem hiding this comment.
A function prop type is denoted with func: onClick: React.PropTypes.func,
There was a problem hiding this comment.
I had updated to "PropTypes.func" and it still gave me an error. Found out it was a result of another error. SO, fixed all that 👍
| </fieldset> | ||
| </Form> | ||
| ) | ||
| export default class SignInForm extends React.Component { /** |
There was a problem hiding this comment.
@n8mellis should this be broken down in to smaller components, or should it be handled as a part of SignInForm component - button texts, show/hide password toggle?
… vanguyen/dev Conflicts: styleguide/containers/FormControlsContainer.jsx styleguide/containers/FormTemplatesContainer.jsx
|
@n8mellis completed the templates - updated to latest docs structure (thanks @mstanaland !) please review |
3 participants
Adding in the FormTemplateContainer. Updated some of the field control components used in the prebaked/default Form templates. Only NameForm and AddressForm for now.