-
Notifications
You must be signed in to change notification settings - Fork 64
Conversation
@kopepasah
1 & 5 are the only real bugs. 2-4 are minor design flaws. In general, this is a great start! Really cool to see you pull together this particular implementation. I'm interested in whether you still think it's a good idea doing this as you mentioned concern with that it departs from the native controls. |
@RobStino addressed numbers 1 and 5 in the two new commits. |
@kopepasah |
@RobStino This is a known issue in Gutenberg. WordPress/gutenberg#11791 |
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.
Great feature! Some minor suggestions included here.
@@ -38,5 +38,7 @@ | |||
"uglifyjs-webpack-plugin": "^1.2.7", | |||
"webpack": "^3.11.0" | |||
}, | |||
"dependencies": {} | |||
"dependencies": { | |||
"color": "^3.1.0" |
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.
Is this required? Isn't a colour picker included in WordPress by default? https://github.com/WordPress/gutenberg/tree/master/packages/components/src/color-picker
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.
@lukecarbis this package is not the color picker, but rather used to adjust the darkness of the border in the select color display, this way the border can be a similar color variation.
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.
It would be really nice if we could find a workaround for this without introducing a third-party dependency (our only one!).
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.
We could use tinycolor
, which is used in WordPress, but I did not see it exposed as usable. So it would have to be imported too.
Do you see an issue with introducing dependencies?
'sanitize' => 'sanitize_textarea_field', | ||
) | ||
); | ||
} |
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.
There should also be a default
setting.
help={ field.help } | ||
className="block-lab-color-picker" | ||
> | ||
<span className="components-color-picker__alpha block-lab-color-picker__current-color" style={ { |
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.
Could this be replaced with the ColorIndicator
component? https://github.com/WordPress/gutenberg/tree/master/packages/components/src/color-indicator
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.
It could, but would require some adjusting non-the-less. Do you see any advantages of using the component over the current implementation?
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.
Relying on core components means less maintainability.
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.
While I do agree, in this case we are only referencing a span and, even if we did use the component, we still need the wrapper component and style adjustments (which the core component does not allow to alter on the component level).
Adds a color picker control.