New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extensibility: Define customClassname behavior as filtered blocks support #3472

Merged
merged 7 commits into from Nov 17, 2017

Conversation

Projects
None yet
3 participants
@youknowriad
Contributor

youknowriad commented Nov 14, 2017

This follows the work done in #3318 and uses the same extensibility APIs to define the custom className behavior.

Unlike the anchor, this customClassname is opt-out. to opt-out of if, you need to add the following code to the block settings:

supports: {
  customClassName: false
}

This surfaces some issues with the anchor definition:

  • This update the extraProps filter by passing the original element props to the first filter. This is necessary because we have to append the custom className to the existing className prop.

  • we can't call cloneElement on element fragments which break the anchor/classname filters if you use both on a single block. To resolve this I introduced a extendElement helper. It can be improved IMO, open to suggestions on how to handle this better.

  • This also separates the generatedClassName from the customClassName to allow simple blocks like heading, paragraphs to support custom class names without having a generated one which was not possible. The generatedClassName bahavior could also be refactored to use the extensibility API.

  • This removes the "advanced controls" panel, We could provide it back by grouping these two extensions into a unique one.

Testing instructions

  • Try adding a custom classname on a heading block
  • Try an anchor on the heading block.
  • Save and refresh.
  • You should still see these values populated properly

@youknowriad youknowriad self-assigned this Nov 14, 2017

@youknowriad youknowriad requested review from gziolo and aduth Nov 14, 2017

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Nov 14, 2017

Member

This removes the "advanced controls" panel, We could provide it back by grouping these two extensions into a unique one.

We can bring it back using the approach I propose in #3475 :)

Member

gziolo commented Nov 14, 2017

This removes the "advanced controls" panel, We could provide it back by grouping these two extensions into a unique one.

We can bring it back using the approach I propose in #3475 :)

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Nov 14, 2017

Codecov Report

Merging #3472 into master will increase coverage by 0.03%.
The diff coverage is 90.9%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3472      +/-   ##
==========================================
+ Coverage   34.36%   34.39%   +0.03%     
==========================================
  Files         261      270       +9     
  Lines        6743     6885     +142     
  Branches     1229     1258      +29     
==========================================
+ Hits         2317     2368      +51     
- Misses       3734     3811      +77     
- Partials      692      706      +14
Impacted Files Coverage Δ
blocks/hooks/anchor.js 80% <ø> (ø) ⬆️
blocks/library/more/index.js 22.22% <ø> (ø) ⬆️
editor/components/block-inspector/index.js 14.28% <ø> (ø) ⬆️
blocks/library/html/index.js 16.66% <ø> (ø) ⬆️
blocks/library/shortcode/index.js 40% <ø> (ø) ⬆️
blocks/api/factory.js 86.48% <ø> (-0.7%) ⬇️
blocks/api/parser.js 98% <ø> (-0.08%) ⬇️
blocks/api/serializer.js 98.18% <100%> (ø) ⬆️
components/slot-fill/fill.js 88% <100%> (+13%) ⬆️
components/slot-fill/slot.js 79.16% <100%> (+12.5%) ⬆️
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update afd2627...5c38f6a. Read the comment docs.

codecov bot commented Nov 14, 2017

Codecov Report

Merging #3472 into master will increase coverage by 0.03%.
The diff coverage is 90.9%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3472      +/-   ##
==========================================
+ Coverage   34.36%   34.39%   +0.03%     
==========================================
  Files         261      270       +9     
  Lines        6743     6885     +142     
  Branches     1229     1258      +29     
==========================================
+ Hits         2317     2368      +51     
- Misses       3734     3811      +77     
- Partials      692      706      +14
Impacted Files Coverage Δ
blocks/hooks/anchor.js 80% <ø> (ø) ⬆️
blocks/library/more/index.js 22.22% <ø> (ø) ⬆️
editor/components/block-inspector/index.js 14.28% <ø> (ø) ⬆️
blocks/library/html/index.js 16.66% <ø> (ø) ⬆️
blocks/library/shortcode/index.js 40% <ø> (ø) ⬆️
blocks/api/factory.js 86.48% <ø> (-0.7%) ⬇️
blocks/api/parser.js 98% <ø> (-0.08%) ⬇️
blocks/api/serializer.js 98.18% <100%> (ø) ⬆️
components/slot-fill/fill.js 88% <100%> (+13%) ⬆️
components/slot-fill/slot.js 79.16% <100%> (+12.5%) ⬆️
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update afd2627...5c38f6a. Read the comment docs.

@@ -58,7 +58,7 @@ describe( 'anchor', () => {
it( 'should do nothing if the block settings do not define anchor support', () => {
const attributes = { anchor: 'foo' };
const extraProps = addSaveProps( blockSettings, attributes );
const extraProps = addSaveProps( {}, blockSettings, attributes );

This comment has been minimized.

@gziolo

gziolo Nov 14, 2017

Member

Good catch, this is the issue with testing only for non-existence. It is very easy to satisfy 😃

@gziolo

gziolo Nov 14, 2017

Member

Good catch, this is the issue with testing only for non-existence. It is very easy to satisfy 😃

Show outdated Hide outdated package-lock.json
<Slot name="Inspector.Controls" />
<BlockInspectorAdvancedControls key={ `advanced-controls-${ selectedBlock.uid }` } />
</div>
<Slot name="Inspector.Controls" />

This comment has been minimized.

@gziolo

gziolo Nov 14, 2017

Member

It conflicts with the changes I propose with #3475, but we can resolve that later if necessary.

@gziolo

gziolo Nov 14, 2017

Member

It conflicts with the changes I propose with #3475, but we can resolve that later if necessary.

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Nov 14, 2017

Member

This was super fast refactoring 🚀 It looks really solid. I left a few comments. Some of them should be really noted in the preceding PR, but I missed them. I don't think I discovered any blockers. I will give it a round of testing tomorrow unless @aduth accepts those changes earlier :)

Member

gziolo commented Nov 14, 2017

This was super fast refactoring 🚀 It looks really solid. I left a few comments. Some of them should be really noted in the preceding PR, but I missed them. I don't think I discovered any blockers. I will give it a round of testing tomorrow unless @aduth accepts those changes earlier :)

Show outdated Hide outdated blocks/api/serializer.js
Show outdated Hide outdated blocks/api/serializer.js
Show outdated Hide outdated blocks/hooks/utils.js
@@ -33,6 +33,10 @@ registerBlockType( 'core/html', {
supportHTML: false,
supports: {
customClassName: false,

This comment has been minimized.

@aduth

aduth Nov 14, 2017

Member

Can we now drop the className: false from this and other revised blocks?

@aduth

aduth Nov 14, 2017

Member

Can we now drop the className: false from this and other revised blocks?

This comment has been minimized.

@youknowriad

youknowriad Nov 16, 2017

Contributor

We can't, it controls the generated className which we should address separately.

@youknowriad

youknowriad Nov 16, 2017

Contributor

We can't, it controls the generated className which we should address separately.

Show outdated Hide outdated package-lock.json
Show outdated Hide outdated blocks/hooks/utils.js
Show outdated Hide outdated blocks/hooks/utils.js
Show outdated Hide outdated blocks/hooks/custom-classname.js
@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Nov 16, 2017

Member

I added 3d54d33 which is a dirty hack to make rerender of the sidebar toolbar to be predictable when transforming block between different types. See the following screencast to learn about the issue:

class-additional

Without force prop it renders class name field as a first item in the sidebar when transforming it. The issue here is that React reuses existing fill and therefore it's always present on the list of children as the first item. We need to find a more elegant solution that will cover all similar use cases.

Member

gziolo commented Nov 16, 2017

I added 3d54d33 which is a dirty hack to make rerender of the sidebar toolbar to be predictable when transforming block between different types. See the following screencast to learn about the issue:

class-additional

Without force prop it renders class name field as a first item in the sidebar when transforming it. The issue here is that React reuses existing fill and therefore it's always present on the list of children as the first item. We need to find a more elegant solution that will cover all similar use cases.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Nov 16, 2017

Member

Pushed 73c3e30 as an attempt to resolve the ordering issue. Essentially, we were previously unable to rely on the order of fills because the behavior of registration was simply appending. In the above example, when switching block type, the base inspector controls for the original block type unmount, but the class name field does not (because it is common to both). So when the next block type inspector controls render, they are appended after the class name field.

With 73c3e30, we add order tracking to fills, both when it initially mounts, and after any other fill within that slot unmounts, assuming that updates will occur in the expected render order. Then the slot renders using fills ordered by this occurrence.

Member

aduth commented Nov 16, 2017

Pushed 73c3e30 as an attempt to resolve the ordering issue. Essentially, we were previously unable to rely on the order of fills because the behavior of registration was simply appending. In the above example, when switching block type, the base inspector controls for the original block type unmount, but the class name field does not (because it is common to both). So when the next block type inspector controls render, they are appended after the class name field.

With 73c3e30, we add order tracking to fills, both when it initially mounts, and after any other fill within that slot unmounts, assuming that updates will occur in the expected render order. Then the slot renders using fills ordered by this occurrence.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Nov 16, 2017

Contributor

Team effort ❤️

Contributor

youknowriad commented Nov 16, 2017

Team effort ❤️

@@ -61,6 +61,7 @@ class SlotFillProvider extends Component {
this.fills[ name ],
instance
);
this.resetFillOccurrence( name );

This comment has been minimized.

@gziolo

gziolo Nov 16, 2017

Member

I think this is the most important line. It causes the key to update in slot implementation. 💯

@gziolo

gziolo Nov 16, 2017

Member

I think this is the most important line. It causes the key to update in slot implementation. 💯

<Filler name="egg" key="first" text="first" />,
<Filler name="egg" key="second" text="second" />,
],
} );

This comment has been minimized.

@gziolo

gziolo Nov 16, 2017

Member
element.setProps( {
	children: [
		<Slot name="egg" key="slot" />,
 		<Filler name="egg" key="second" text="second" />,
 		<Filler name="egg" key="first" text="first" />,
 	],
 } )

I tried the following instead of line 122 and it outputs <div>secondfirst</div>. Which means you can use still use concatChildren, because it doesn't care about the keys passed. However it is predictable because it resets occurences for every slot whenever any fill gets removed.

If I add the same code after line 128, it still renders <div>firstsecond</div> because occurrences are not modified.

@gziolo

gziolo Nov 16, 2017

Member
element.setProps( {
	children: [
		<Slot name="egg" key="slot" />,
 		<Filler name="egg" key="second" text="second" />,
 		<Filler name="egg" key="first" text="first" />,
 	],
 } )

I tried the following instead of line 122 and it outputs <div>secondfirst</div>. Which means you can use still use concatChildren, because it doesn't care about the keys passed. However it is predictable because it resets occurences for every slot whenever any fill gets removed.

If I add the same code after line 128, it still renders <div>firstsecond</div> because occurrences are not modified.

@gziolo

gziolo approved these changes Nov 17, 2017

All issues were resolved. Great team work! 🚀

I'll give it another round of testing and merge 🎉

@gziolo gziolo merged commit c4118f2 into master Nov 17, 2017

3 checks passed

codecov/project 34.39% (+0.03%) compared to afd2627
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@gziolo gziolo deleted the update/custom-classname branch Nov 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment