Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Blocks: Move logic for generating class name to BlockEdit component #4009

Merged
merged 5 commits into from
Dec 18, 2017
Merged

Blocks: Move logic for generating class name to BlockEdit component #4009

merged 5 commits into from
Dec 18, 2017

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Dec 14, 2017 •
edited
Loading

Description

Raised in #3741 (comment):

Can we move it to EditBlock? I think it's an internal thing which shouldn't be part of this and the original component.

When working on this I discovered that preview for invalid EditBlock doesn't get updated with all supports options like anchor in case of heading. This PR fixes it.

This was always the case for Reusable Block. This PR fixes it by sharing the same logic within <BlockEdit /> component.

How Has This Been Tested?

Manually and unit tests.

Test steps:

  1. Open an existing post or page.
  2. Make sure it loads properly and all classes are assigned properly to blocks when in edit mode.
  3. Make sure there are no errors on the console triggered by block validation.
  4. Edit/add Heading block - add anchor and class name.
  5. Make sure you can see both of them in the block's HTML code.
  6. Open HTML mode for this block.
  7. Make sure HTML contains class name and anchor name added in the previous steps.
  8. Add something to the HTML code which is going to trigger validation error - you can append <x> at the end.
  9. Make sure you see the error state for block and preview of the block. Make sure it contains class name and anchor name.
  10. Edit/add Custom HTML block. Add some code and append something that will trigger the validation error after page reload. You can type only <x>
  11. Save your document and refresh the page.
  12. You should see the validation error for the block and preview of the HTML code.
  13. Repeat the same steps for Reusable Block.

@noisysocks, can you double check it works properly with Reusable blocks?

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows has proper inline documentation.

@gziolo gziolo added [Feature] Blocks Overall functionality of blocks [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement. labels Dec 14, 2017
@gziolo gziolo self-assigned this Dec 14, 2017
@gziolo gziolo removed the [Status] In Progress Tracking issues with work in progress label Dec 15, 2017
@gziolo gziolo force-pushed the update/generated-class-name-edit-block branch from 179905a to 83dfe66 Compare December 15, 2017 17:58
youknowriad
youknowriad reviewed Dec 18, 2017
View reviewed changes
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this introduced a regression in Reusable blocks. To test save a pullquote as a reusable block, and reload, it'll lose its styles.

@gziolo gziolo mentioned this pull request Dec 18, 2017
Closed
@gziolo
Copy link
Member Author

gziolo commented Dec 18, 2017

It looks like this introduced a regression in Reusable blocks. To test save a pullquote as a reusable block, and reload, it'll lose its styles.

Good catch @youknowriad. you opened a can of worms, see #3967 (comment) :)

@gziolo gziolo force-pushed the update/generated-class-name-edit-block branch from 83dfe66 to f2bcebb Compare December 18, 2017 10:15
gziolo
gziolo commented Dec 18, 2017
View reviewed changes
blocks/library/block/index.js
@@ -167,6 +160,10 @@ registerBlockType( 'core/block', {
},
},

supports: {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let implementing block decide whether the custom class name is supported.

youknowriad
youknowriad reviewed Dec 18, 2017
View reviewed changes
blocks/library/block/index.js
@@ -15,7 +14,8 @@ import { __ } from '@wordpress/i18n';
/**
* Internal dependencies
*/
import { getBlockType, registerBlockType, hasBlockSupport, getBlockDefaultClassname } from '../../api';
import BlockEdit from '../../block-edit';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Is this exposed? How do we use this component when moved to the editor module :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is exposed: https://github.com/WordPress/gutenberg/blob/master/blocks/index.js#L21.

youknowriad
youknowriad reviewed Dec 18, 2017
View reviewed changes
editor/components/block-list/block.js Outdated
attributes: block.attributes,
className,
} ),
getSaveElement( blockType, block.attributes ),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't we have a React warning here about the missing key?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't notice any. See example:

screen shot 2017-12-18 at 11 40 15

I will add one to be on the safe side.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed with 3650811.

@gziolo
Copy link
Member Author

gziolo commented Dec 18, 2017

@youknowriad it is ready for another look.

youknowriad
youknowriad approved these changes Dec 18, 2017
View reviewed changes
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 Nice refactoring

@gziolo gziolo merged commit 227a93f into master Dec 18, 2017
@gziolo gziolo deleted the update/generated-class-name-edit-block branch December 18, 2017 11:09
@gziolo gziolo mentioned this pull request Dec 18, 2017
Closed
18 tasks
@snyk-bot snyk-bot mentioned this pull request Dec 29, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@youknowriad youknowriad youknowriad approved these changes

@aduth aduth Awaiting requested review from aduth

@noisysocks noisysocks Awaiting requested review from noisysocks

Assignees

@gziolo gziolo

Labels
[Feature] Blocks Overall functionality of blocks [Type] Enhancement A suggestion for improvement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gziolo @youknowriad