Block API: consolidate the API to declare className and anchor support #2507
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2507 +/- ##
==========================================
- Coverage 28.7% 28.59% -0.12%
==========================================
Files 167 167
Lines 5103 5144 +41
Branches 849 865 +16
==========================================
+ Hits 1465 1471 +6
- Misses 3086 3107 +21
- Partials 552 566 +14
Continue to review full report at Codecov.
|
youknowriad
changed the title
There was a problem hiding this comment.
Can we use "supports" instead of "support" for consistency with related PHP APIs (register_post_type, register_taxonomy)
Doesn't behavior of generated class name depend on where we decide with #2035 ?
This is part of why I wanted these to be opt-in only. There's too much fragmentation (generated vs. custom vs. default class names), inconsistency (auto-application in save but not edit), and variation (defaulting to true vs. false, other value types like a default class string).
It'd be so nice if this were just:
supports: [ 'className', 'anchor' ],
Then assigning generated class name if developer decides to, not automatically.
| @@ -64,20 +65,22 @@ class BlockInspectorAdvancedControls extends Component { | |||
| render() { | |||
| const { selectedBlock, post } = this.props; | |||
| const blockType = getBlockType( selectedBlock.name ); | |||
| if ( false === blockType.className && ! blockType.supportAnchor ) { | |||
| const supportAnchor = blockType.support && !! blockType.support.anchor; | |||
| const supportCustomClassname = isUndefined( get( blockType, 'support.className' ) ) || blockType.support.className; | |||
There was a problem hiding this comment.
Seems like the previous implementation was simpler? false !== support.className
There was a problem hiding this comment.
This does not account of undefined support value
blocks/library/more/index.js
Outdated
| @@ -21,7 +21,10 @@ registerBlockType( 'core/more', { | |||
|
|
|||
| useOnce: true, | |||
|
|
|||
| className: false, | |||
| support: { | |||
| generatedClassname: false, | |||
There was a problem hiding this comment.
The camel-casing of className is inconsistent here (Classname vs. className). I think we ought to call this generatedClassName.
Aside: I like import classnames from 'classnames'; only because camel-casing in the case of NPM dependencies should be on the basis of dash-delimiters (i.e. classNames if it were class-names).
This is a good API indeed. I'm just trying to accomodate our current needs. @mtias wanted the classNames to stay opt-out. And dropping the generatedClassname, I also personally agree. Once the decision is made, we shall drop the support key. |
|
At its current state, this fixes issues like #2552 should we merge it or wait until a decision has been made about the opt-in className? |
|
For #2552, couldn't we put together a simpler pull request to remove this line? gutenberg/blocks/library/heading/index.js Line 30 in 6143210 |
|
@aduth no, because we don't want the generated className for headings |
|
Any update, decision on this? I think this is still better than what we currently have. |
|
Still very conflicted about this. Left some related comments at #2474 (comment). |
|
@aduth I agree that it's a good opportunity to proof the extensibility patterns, I think this will change the current behaviour a bit, but probably for a better tradeoff. I'd be happy to poc on that if no one is |
|
closing this, we'll try to build this using an extensibility API |
related to #2035
This exposes the current property in the blockAPI:
Instead of the current
classNameandsupportAnchor.This also splits
classNameattribute into twogeneratedClassnameandclassName. I did this because it makes sense to allow a custom classname for some blocks without generating an automattic one. For example: paragraphs, heading, lists