Add inspector range control #1144
Conversation
…ype attribute to ‘text’ to inherit the WordPress CSS styles.
| const id = 'inspector-range-control-' + instanceId; | ||
|
|
||
| return ( | ||
| <div className="blocks-inspector-control"> |
There was a problem hiding this comment.
this breaks our css guidelines https://github.com/WordPress/gutenberg/blob/master/docs/coding-guidelines.md#css
The className here should be "blocks-range-control" and the subclassNames "blocks-range-control__label". This forces us to think in shared components instead of shared classNames. We could probably create a generic BaseControl responsible of showing the label and the surroundings.
blocks/library/gallery/index.js
Outdated
| @@ -120,8 +121,7 @@ registerBlockType( 'core/gallery', { | |||
|
|
|||
| edit( { attributes, setAttributes, focus, id } ) { | |||
There was a problem hiding this comment.
Should we remove id here since it's no longer used?
| import './style.scss'; | ||
|
|
||
| function TextControl( { label, value, instanceId, onChange } ) { | ||
| function TextControl( { label, value, instanceId, onChange, ...attributes } ) { |
There was a problem hiding this comment.
Maybe props instead of attributes just to respect React's wording
* Remove unused id. * Conform to CSS guideline naming conventions.
|
Gone ahead and addressed all the notes. |
| const id = 'inspector-text-control-' + instanceId; | ||
| const type = props.type || 'text'; |
There was a problem hiding this comment.
Could we use default values + destructuring instead?
TextControl( { label, value, instanceId, onChange, type = 'text', ...props } ) {
|
I'm seeing some spaces/tabs mixed usages in different files, |
|
Done and done. |
|
I'm seeing a small style issue (The number is not correctly aligned and maybe the control should be 100% width) but we have the same issue on master. I'll merge when the tests completes.
|
|
Thanks a lot for working on this |
| <label className="blocks-text-control__label" htmlFor={ rangeId }>{ __( 'Columns' ) }</label> | ||
| <input id={ rangeId } type="range" min="1" max={ Math.min( MAX_COLUMNS, images.length ) } value={ columns } onChange={ setColumnsNumber } /> | ||
| <span>{columns}</span> | ||
| <RangeControl label={ __( 'Columns' ) } value={ columns } onChange={ setColumnsNumber } min="1" max={ Math.min( MAX_COLUMNS, images.length ) } /> |
There was a problem hiding this comment.
Might have helped readability to split these props across multiple lines (very long line).
There was a problem hiding this comment.
👍 I missed it. We should probably add an eslint rule for this.
As suggested on #1135 (comment), this separates the range control into its own component.
I'm more fluent in Vue.js and have never touched React, so let me know if there's anything that looks wrong.
This pull request does the following:
.blocks-inspector-control-*classes.TextControlcomponent.textso it inherits the WordPress CSS styles.RangeControlcomponent and tweaked the style to improve the grid.