Add all input inspector controls #1202
Conversation
|
A followup suggestion would be to export all of the core control types similar to this file: Then it would be possible to more easily import each control. For example: |
|
It looks like all the controls more or less, have a wrapper div, a label and an input (or similar). <BaseControl label={ "my label" } id={ id }>
<input id={ id } />
</BaseControl> |
| */ | ||
| import './style.scss'; | ||
|
|
||
| function BaseControl( { id, label, classes, children, ...props } ) { |
There was a problem hiding this comment.
Should we rename classes as className for consistency with React Props and our other components?
|
|
||
| return ( | ||
| <BaseControl label={ heading } id={ id }> | ||
| <label className="blocks-checkbox-control__label" htmlFor={ id }> |
There was a problem hiding this comment.
Should we drop the label here? It's already added by BaseControl?
There was a problem hiding this comment.
This one in particular is for the label beside the checkbox. So in my screenshot it is the "Option" text. There is an optional label (Coming from the BaseControl) with the prop 'heading' which is "Checkbox Control" in my screenshot.
There was a problem hiding this comment.
I see! I'm wondering if it's ok for an input to have two labels? Maybe we should avoid the label prop for this control and add it explicitly here using another tag.
There was a problem hiding this comment.
So remove the heading label prop, and then change the prop for the checkbox to something else. How about optionLabel or checkboxLabel?
There was a problem hiding this comment.
Well! According to the HTML Documentation, we can have two labels for the same input. So, let's leave it as is
More than one LABEL may be associated with the same control by creating multiple references via the for attribute.
| return ! isEmpty( options ) && ( | ||
| <BaseControl label={ label } id={ id }> | ||
| <div className="blocks-radio-control"> | ||
| { options.map( ( option, i ) => |
There was a problem hiding this comment.
Should we rename i to index to avoid 1 character variables?
| <BaseControl label={ label } id={ id }> | ||
| <div className="blocks-radio-control"> | ||
| { options.map( ( option, i ) => | ||
| <label key={ option.value } htmlFor={ ( id + '-' + i ) }> |
There was a problem hiding this comment.
I think for accessibility reasons, we're avoiding nesting inputs in labels and always using htmlFor as an alternative.
|
|
||
| return ! isEmpty( options ) && ( | ||
| <BaseControl label={ label } id={ id }> | ||
| <div className="blocks-radio-control"> |
There was a problem hiding this comment.
Is the div necessary here? what if we pass className to the BaseControl component? (same for all other controls)
|
I've gone ahead and addressed feedback. |
| */ | ||
| import './style.scss'; | ||
|
|
||
| function BaseControl( { id, label, className, children, ...props } ) { |
There was a problem hiding this comment.
I'm thinking we should drop the ...props here until we explicitly need it.
| const onChangeValue = ( event ) => onChange( event.target.value ); | ||
|
|
||
| return ! isEmpty( options ) && ( | ||
| <BaseControl label={ label } id={ id } className={ [ 'blocks-radio-control' ] }> |
There was a problem hiding this comment.
Might be a tad simpler to pass className as string, i.e. className="blocks-radio-control"
|
Anything I can do myself to get this moving forward? Also note #1198 which is somewhat of a prerequisite to this pull request. |
|
If you can just rebase, it'll be awesome :) |
|
See #1278. |
This is a followup to #1198, and adds all the basic controls (input field types).
In particular it adds:
I've also made a few style tweaks to existing controls for styling consistencies among all the control types.
Here is a screenshot of all the controls in action: