feat(Form): base implementation #353
Conversation
mnajdova
requested review from
dvdzkwsk,
kuzhelov,
levithomason and
miroslavstastny
as code owners
|
Can you remove children lines - they're just cut and paste with old format. |
|
Side or size? |
|
Loading indicator - is that specified in theme? or how would user customize? |
|
Children removed, side -> size fixed. The loader indicator will be indeed styled by the theme. We might decide for adding Loading component in the future and make this shorthand, but in the beginning this will be some slot inside the Layout with some styles defined in the component's styles. |
UpdateThe Form is implemented using the Grid component with default QuestionDo we want to support fields shorthand inside the form for all elements inside? If we allow it would be easy to apply some props to all fields inside (for example the size property which will allow different sizing to all elements inside the form). We can achieve the same using context, but as this is not used anywhere so far wasn't sure if we want to go on that path. |
Codecov Report
@@ Coverage Diff @@
## master #353 +/- ##
==========================================
+ Coverage 89.94% 90.55% +0.61%
==========================================
Files 64 67 +3
Lines 1253 1334 +81
Branches 162 169 +7
==========================================
+ Hits 1127 1208 +81
Misses 123 123
Partials 3 3
Continue to review full report at Codecov.
|
-refactored the styling for the inline prop
…ze prop of the label
src/components/Form/FormField.tsx
Outdated
| // No Control | ||
| // ---------------------------------------- | ||
|
|
||
| if (_.isNil(control)) { |
There was a problem hiding this comment.
nit: seems that it could be simplified to if (control) ... without forfeiting correctness - to make this check be consistent with other ones used in this file
src/components/Form/Form.tsx
Outdated
| className: PropTypes.string, | ||
|
|
||
| /** The columns of the grid with a space-separated list of values. The values represent the track size, and the space between them represents the grid line. */ | ||
| columns: PropTypes.oneOfType([PropTypes.string, PropTypes.number]), |
There was a problem hiding this comment.
I would rather consider to remove layout-related props from the Form's API for now, as am pretty sure that we would be able to address quite wide set of scenarios by
- introducing base 'vertical form' layout support as a default one
- ability to customize form styles for the client - not that with that it would be possible for the client to provide custom layout styles for each Form.Field element
- potentially add ability to introduce support for horizontal forms
Not saying that for sure we won't need Grid support for form's layout - the only thing that stating is that for now we, probably, don't need it
There was a problem hiding this comment.
This seemed like the easier (for implementation and usage) layouting mechanism support out of the box for the form component. With this by default we are supporting the vertical, multi-column and horizontal layout. We might introduce this as a consequent PR too, after the initial implementation is merged. What are your thoughts on this? And, yes, I would definitelly use the Grid in the background anyway for aligning the elements
There was a problem hiding this comment.
yes - my point is rather about how this layou mechanism would be exposed. And, given that this is a question that should involve some consideration phases, I would not introduce any layout aspects to the API in this PR
src/components/Form/Form.tsx
Outdated
| onSubmit: PropTypes.func, | ||
|
|
||
| /** Automatically show a loading indicator. */ | ||
| loading: PropTypes.bool, |
There was a problem hiding this comment.
emm, not sure that it should be part of the default form interface. To me loading seems to be something that introduces behavioural aspects to the form - and, thus, see this loading aspect to be used as rather decorator for the general Form component.
There was a problem hiding this comment.
Make sense, will remove this prop for now. It seems like a good concept, but in the end introducing this here would then bring out question as why only this component has the loading indicator, so for this moment I agree that it should not be part of the Form component.
src/components/Form/Form.tsx
Outdated
| action: PropTypes.string, | ||
|
|
||
| /** The size for the Text component */ | ||
| size: PropTypes.oneOf(['smallest', 'smaller', 'small', 'medium', 'large', 'larger', 'largest']), |
There was a problem hiding this comment.
not sure that it should be exposed for Form component, especially given that this prop, in fact, defines Text size (we should ensure prop types consistency here, by the way then). Pretty sure that it was added for reason - could you, please, suggest which case you've tried to address with that?
There was a problem hiding this comment.
The idea for adding this prop was providing out of the box sizing mechanism of all items inside the form. For example, if the user wants the form to have bigger size then regular, applying only one prop on the form, should scale all elements inside (for now it is just the Text components - which are used for the labels, but we should further adjust the sizes of the input and other elements which are going to be used as FormFields) If this property is not added here, then the user should be himself responsible for correct sizing of all elements inside the Form. Please see the latest commit on the PR, which is introducing the SizeContext that is used inside the FormField component.
src/components/Form/FormField.tsx
Outdated
| className?: string | ||
| label?: ShorthandValue | ||
| renderLabel?: ShorthandRenderFunction | ||
| control?: React.ReactType<any> |
There was a problem hiding this comment.
yes, agree, this should considered to be a shorthand.
There was a problem hiding this comment.
The logic for generating the control is a bit tricky. The control can be some component (we still don't have all components that can appear in the form implemented) or some HTML element (checkbox, button etc.). This is why this is just a type, and for consistency all properties available in the FormField are applied to the control element. What would be your proposal for using the shorthand in this case? Would be use the shorthand as one specific component and then all other components will always send the <Component ...props /> or some function and be responsible for applying all props inside that component?
There was a problem hiding this comment.
Here is the thing that I am concerned about - how we would be able to customize styles of this component? Yes, while it would be possible (with current approach) to pass styles to Form.Field, and those, if I've understood you correctly, will be passed to content element - for regular HTML element, however, we will have a problem that we would provide unknown styles attribute. This is the reason why I see this boundary should be clearly advertised - specifically, by providing the following semantics to the client: here is the control slot, please, provide all the necessary component props and style customizations there.
So, I would suggest to use createHTMLInput method from factories.tsx as a value for content - and even given that we have some problems with its implementation now, with this being fixed we should end up with correct implementation for Form.Field as well
There was a problem hiding this comment.
The control is converted to shorthand, and controlType prop is added that is the previous control prop. Depending on the controlType, different slot shorthand factories are created. @Bugaa92 I need your input whether this is the right approach. And yeah, I will need to add some custom tests for this prop, after we decide on the API used.
|
I do like the proposal, totally agree with structuring Have left some comments, but those are, mainly, necessary to clarify some bits. |
src/components/Form/Form.tsx
Outdated
| }): React.ReactNode { | ||
| const { as, rows, columns, action, children, content, size } = this.props | ||
| return ( | ||
| <SizeContext.Provider value={{ size }}> |
There was a problem hiding this comment.
would rather try to avoid this in general, and certainly at the early stages. Could we list the cases where we need this sizing, and after that think about whether there are alternative ways to achieve the same goal?
There was a problem hiding this comment.
This was the reason why I added the sizing mechanism: #353 (comment) I agree that we may want to address this in some consequent PR, and not the initial implementation.
There was a problem hiding this comment.
Reverted the changes for the sizing mechanism from this PR. We can come back to these in some consequent PR.
kuzhelov
added
needs author feedback
-alphabetical order of the props interfaces and prop types -improved description of the props -rearranged the content of the FormField
# Conflicts: # CHANGELOG.md # test/specs/components/Slot/Slot-test.ts
kuzhelov
added
🚀 ready for review
and removed
needs author feedback
src/components/Input/Input.tsx
Outdated
| }, | ||
| render: renderWrapper, | ||
| }) | ||
| return ( |
There was a problem hiding this comment.
let's not make this huge amount of changes just for the fact that we cannot use
the issue was that I forgot to add children as handled prop to the Input so they are passed as undefined to the Slot.create method; better fix is to just add:
/**
* Used to set content when using childrenApi - internal only
* @docSiteIgnore
*/
children: PropTypes.node,to the propTypes object in this file and
children?: ReactChildrento the IInputProps interface
There was a problem hiding this comment.
Yep, this solved the issue. Thanks!
| const fields = [ | ||
| { | ||
| label: 'First name', | ||
| control: { as: Input }, |
There was a problem hiding this comment.
could we provide either Input or input as a default value - for the sake of it not being necessary for the client to always specify the underlying type?
There was a problem hiding this comment.
We can provide the control to be by default {as: Input} but if the user wants to override it, would have to specify the as again, otherwise we would need to handle all different types of control (object, component etc) Would this be enough?
| formFieldImplementsShorthandProp('message', Text) | ||
| formFieldImplementsShorthandProp('control', Slot) | ||
|
|
||
| it('renders the control provided in the controlType prop', () => { |
There was a problem hiding this comment.
now this could be renamed :) Same applies to the next test
There was a problem hiding this comment.
This will be renamed, no need for the second now that the control is a regular shorthand :)
-small refactoring for addressing PR comments
|
|
||
| public static defaultProps = { | ||
| as: 'div', | ||
| control: { as: Input }, |
src/index.ts
Outdated
| @@ -27,6 +27,8 @@ export { default as Status } from './components/Status' | |||
| export { default as TabBehavior } from './lib/accessibility/Behaviors/Tab/tabBehavior' | |||
| export { default as TabListBehavior } from './lib/accessibility/Behaviors/Tab/tabListBehavior' | |||
| export { default as Text } from './components/Text' | |||
| export { default as Form } from './components/Form' | |||
There was a problem hiding this comment.
nit: those two could be grouped together export { default as Form, FormField } from './components/Form'
This PR aims to create the base implementation for the Form component. Here is a short RFC for the proposed API.
Form
This component will be used for displaying a set of related user input fields in a structured way. In the implementation the Form component will use the Grid component for customizing the appearance of the fields inside.
TODO
API Proposal
as: defaultValue: 'form'
An element type to render as (string or function).
className
Additional classes.
action
The HTML form action.
onSubmit
The HTML form submit handler.
fields
Array shorthand for the fields inside the form.
message: Segment shorthand prop
Will show a message inside the form (error, success, warning etc.)
UPDATE: the message prop will be implemented to the Form.Field component instead of here.
size: enum: sizes
Size of the text and elements inside the form. Should be consistent with the Text's sizes.
UPDATE: this will be addressed in a consequent PR, as we still haven't decided on the way it should be implemented.
loading: PropTypes.bool
Automatically show a loading indicator.
UPDATE: after discussion, decided that this prop will not be introduced. See comment: #353 (comment)
styles: ComponentPartStyle
variables?: ComponentVariablesInput
Form.Field
A field is a form element containing a label and an input.
as
An element type to render as (string or function).
className
Additional classes.
content: customPropTypes.contentShorthand
Shorthand for primary content.
control: customPropTypes.some([PropTypes.func, PropTypes.oneOf(['button', 'input', 'select', 'textarea'])])
A form control component (i.e. Input, RadioGroup) or HTML tagName (i.e. 'input').
Extra FormField props are passed to the control component.
Mutually exclusive with children.
error: PropTypes.bool
Individual fields may display an error state.
UPDATE: this prop will be added in a consequent PR that will address the validation aspects.
inline: PropTypes.bool
A field can have its label next to instead of above it.
label: Text with default as='label' shorthand
Mutually exclusive with children.
required: PropTypes.bool
A field can show that input is mandatory.
styles: ComponentPartStyle
variables?: ComponentVariablesInput
By default the Form field will render the following HTML structure giving label and a input control:
For the components that doesn't need label (like the Button), or are providing the label together with the component the label will be rendered according to that component implementation.