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: Use the Extensibility API to implement the generated className behaviour #3562

Merged
merged 3 commits into from Nov 22, 2017

Conversation

Projects
None yet
4 participants
@youknowriad
Contributor

youknowriad commented Nov 20, 2017

This PR refactors the generated className beahavior to use the extensibility API and consolidate with similar hooks (anchor and custom className).

The question of whether we should keep this or not still remains. An option would be to provide the className as prop to the save function like we do for the edit function. Regardless, I'm personally happy with this implementation better than what we currently have.

@youknowriad youknowriad self-assigned this Nov 20, 2017

@youknowriad youknowriad requested review from mtias, gziolo and aduth Nov 20, 2017

Show outdated Hide outdated blocks/api/serializer.js Outdated
generatedClassName( hooks );
blockSettings = {
name: 'chicken/ribs',

This comment has been minimized.

@gziolo

gziolo Nov 20, 2017

Member

chicken/wings? :trollface:

@gziolo

gziolo Nov 20, 2017

Member

chicken/wings? :trollface:

@gziolo

gziolo approved these changes Nov 20, 2017

This code looks great. There are two places where I think we should update code, but those are minor changes. I tested this code and it works properly, class names are appended to the blocks as before. 👍

I'm accepting this PR, but I would double check with @mtias and/or @aduth if we want to keep this functionality as it is.

Show outdated Hide outdated blocks/library/more/index.js Outdated
Show outdated Hide outdated docs/block-api.md Outdated
Show outdated Hide outdated docs/block-api.md Outdated
Show outdated Hide outdated editor/modes/visual-editor/block.js Outdated
Show outdated Hide outdated blocks/library/more/index.js Outdated
@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Nov 21, 2017

Codecov Report

Merging #3562 into master will increase coverage by <.01%.
The diff coverage is 81.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3562      +/-   ##
==========================================
+ Coverage   36.92%   36.93%   +<.01%     
==========================================
  Files         267      268       +1     
  Lines        6675     6676       +1     
  Branches     1205     1203       -2     
==========================================
+ Hits         2465     2466       +1     
  Misses       3557     3557              
  Partials      653      653
Impacted Files Coverage Δ
blocks/library/list/index.js 6.89% <ø> (ø) ⬆️
blocks/library/shortcode/index.js 40% <ø> (ø) ⬆️
blocks/library/more/index.js 22.22% <ø> (ø) ⬆️
blocks/library/heading/index.js 25% <ø> (ø) ⬆️
blocks/library/html/index.js 16.66% <ø> (ø) ⬆️
blocks/library/paragraph/index.js 29.16% <ø> (ø) ⬆️
editor/components/block-list/block.js 0.74% <0%> (ø) ⬆️
blocks/hooks/index.js 100% <100%> (ø) ⬆️
blocks/hooks/generated-class-name.js 100% <100%> (ø)
blocks/api/serializer.js 98% <100%> (-0.19%) ⬇️
... and 1 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 2bb48d5...8de3e75. Read the comment docs.

codecov bot commented Nov 21, 2017

Codecov Report

Merging #3562 into master will increase coverage by <.01%.
The diff coverage is 81.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3562      +/-   ##
==========================================
+ Coverage   36.92%   36.93%   +<.01%     
==========================================
  Files         267      268       +1     
  Lines        6675     6676       +1     
  Branches     1205     1203       -2     
==========================================
+ Hits         2465     2466       +1     
  Misses       3557     3557              
  Partials      653      653
Impacted Files Coverage Δ
blocks/library/list/index.js 6.89% <ø> (ø) ⬆️
blocks/library/shortcode/index.js 40% <ø> (ø) ⬆️
blocks/library/more/index.js 22.22% <ø> (ø) ⬆️
blocks/library/heading/index.js 25% <ø> (ø) ⬆️
blocks/library/html/index.js 16.66% <ø> (ø) ⬆️
blocks/library/paragraph/index.js 29.16% <ø> (ø) ⬆️
editor/components/block-list/block.js 0.74% <0%> (ø) ⬆️
blocks/hooks/index.js 100% <100%> (ø) ⬆️
blocks/hooks/generated-class-name.js 100% <100%> (ø)
blocks/api/serializer.js 98% <100%> (-0.19%) ⬇️
... and 1 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 2bb48d5...8de3e75. Read the comment docs.

Fixed :)

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Nov 22, 2017

Contributor

Merging this to move foward with the Block versioning (it wiill simplify things in the parser). Granted this could be removed later in favor of explicit className props.

Contributor

youknowriad commented Nov 22, 2017

Merging this to move foward with the Block versioning (it wiill simplify things in the parser). Granted this could be removed later in favor of explicit className props.

@youknowriad youknowriad merged commit e56a73f into master Nov 22, 2017

3 checks passed

codecov/project 36.93% (+<.01%) compared to 2bb48d5
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@youknowriad youknowriad deleted the add/generated-classname-hook branch Nov 22, 2017

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