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.

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

Block library: Standardize file structure of core blocks #14743

Merged
merged 1 commit into from
Apr 2, 2019

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Apr 1, 2019

Description

This is trying to bring the structure of core blocks closer to what is proposed in block registration RFC #13693. In particular, it ensures that:

  • all edit methods are moved to their own file
  • all SVG based icons are moved to their own files

Extracting SVG icons is going to be a good step towards exploring how #14628 could be achieved as all SVG based icons would be located in their own files making it easier to reason how to convert them to the current form from the commonly known file format.

Testing

Ensure all block icons in the inserter display as before for core blocks.

Follow-up task

  • Perform the same refactoring for save methods as well.
  • Investigate what to do about all other client-side properties like transforms or merge.

@gziolo gziolo added [Type] Enhancement A suggestion for improvement. [Type] Code Quality Issues or PRs that relate to code quality [Package] Block library /packages/block-library labels Apr 1, 2019
@gziolo gziolo self-assigned this Apr 1, 2019
@gziolo gziolo force-pushed the update/block-library-structure-polish branch from c225b78 to 6546a64 Compare April 1, 2019 15:01
@gziolo gziolo marked this pull request as ready for review April 1, 2019 15:02
@gziolo gziolo added the [Feature] Block Directory Related to the Block Directory, a repository of block plugins label Apr 1, 2019

return (
<RichText
identifier="values"
Copy link
Member

Choose a reason for hiding this comment

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

Sometimes we seem to set these as constants, other times not. Would it be good to standardise this too? Personally, I feel like the constants are good, but a separate file for it a bit too much.

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, we should both ensure they are provided and figure out what's the best way to encode them. We can't use constants inside block.json files but maybe it would be possible to create ESLint rule which would ensure that identifiers use the existing attribute's name.

Copy link
Member

@ellatrix ellatrix left a comment

Choose a reason for hiding this comment

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

Looks good. Personally I think the contants file is a bit too much, especially if we will have them for all blocks.

@gziolo
Copy link
Member Author

gziolo commented Apr 2, 2019

Looks good. Personally I think the contants file is a bit too much, especially if we will have them for all blocks.

As it turns out in #14770, I figured out that constants don't play well with block.json so I think we will have to remove them altogether to be able to move attributes to block.json. I will revisit the approach for constants when I have a better idea how blocks can consume JSON metadata.

@gziolo gziolo added this to the 5.5 (Gutenberg) milestone Apr 2, 2019
@gziolo gziolo merged commit 495f54c into master Apr 2, 2019
@gziolo gziolo deleted the update/block-library-structure-polish branch April 2, 2019 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block Directory Related to the Block Directory, a repository of block plugins [Package] Block library /packages/block-library [Type] Code Quality Issues or PRs that relate to code quality [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants