-
Notifications
You must be signed in to change notification settings - Fork 812
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
Extensions: Add block editor (Gutenberg) extensions source #11633
Conversation
Thank you for the great PR description! When this PR is ready for review, please apply the Scheduled Jetpack release: April 2, 2019. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not to throw a wrench into this or anything, but in #11312 we had discussed a somewhat different file organization:
12fa4fd#diff-bf97bdc65b5272e6bbde848e059bdd4eR14
I'm fine either way but wanted to point this out. Wouldn't it be cleaner / easier to handle/build to have the jsx in their own folders?Not to throw a wrench into this or anything, but in #11312 we had discussed a somewhat different file organization:
12fa4fd#diff-bf97bdc65b5272e6bbde848e059bdd4eR14
I'm fine either way but wanted to point this out. Wouldn't it be cleaner / easier to handle/build to have the jsx in their own folders?
@jeherve @lezama @sirreal and I discussed folder structure and concluded to keep it as flat (opposed to splitting js+scss files to Gutenberg also has flat structure: Concern was that it would be confusing for complicated blocks with several files and that's also how Jetpack Dashboard files are split. We can iterate once we do have more files/complex blocks (instead of optimizing early). One aspect was to try match "single block plugin" proposal (https://make.wordpress.org/meta/2019/03/08/the-block-directory-and-a-new-type-of-plugin) but that's also something we might not want to optimize too early for, just keep it in mind:
|
This depends on including files to ignores in different places before this is merged: #11639 (we could just bring those changes over to this PR) |
We should update docs file: Maybe something from these files: |
Caution: This PR has changes that must be merged to WordPress.com |
e476f5a
to
928a710
Compare
roccotripaldi, Your synced wpcom patch D25865-code has been updated. |
928a710
to
7b1d412
Compare
|
||
``` | ||
. | ||
└── blockname/ | ||
└── blockname.php ← PHP file where the block and its assets are registered. | ||
└── block-or-plugin-name/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suppose extension could be both plugins and blocks (and filters and whatnot) at the same time, so it might not make sense to optimize this way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be easier for our build tools and for new contributors if everything was in one place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does /extensions/blocks
then make sense and should we just have /extensions
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the things in /blocks
are actually blocks.
#11640 includes additional extensions (the sidebar and category) that aren't blocks. They are not located under blocks.
We can ignore the failing wpcom sync and manually sync the file in a different phabricator. |
Assigning to @jeherve for the review, scheduled for review today. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The blocks seem to build nicely. I do have some questions though.
Should the imported blocks be free of eslint errors? I would assume so since we have Prettier running in Calypso. I can currently see errors like this one, in extensions/shared/frontend-management.js
:
Import individual methods from the Lodash module.
Since switching from jsx to js, we also have a few errors like this one, in extensions/shared/help-message.js
or in extensions/shared/jetpack-plugin-sidebar.js
for example:
'React' must be in scope when using JSX
We'll need to bring this change here when the PR merges: Automattic/wp-calypso#31690 |
3436b67
to
e8aa49e
Compare
I would be tempted to close this PR in favor of #11639 now that the commits are included in that other PR as well, but there are still some questions above that I think would be worth discussing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be working well for me. Let's get #11639 merged and this one soon afterwards to reduce the duplication (and confusion) for developers. I think it's fine to address lint errors etc after the fact.
b5a1dc1
to
c3847b9
Compare
I've just updated and squashed this branch to include the latest Calypso master source (as of Automattic/wp-calypso@0f0ba7b). Ready for review. |
The source for block editor (Gutenberg) extensions was stored in the Calypso repository: https://github.com/Automattic/wp-calypso Move the source into this repository.
c3847b9
to
a59fb1f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:partyparrot: 🚢 IT
Fixes nothing, aside from developer headaches
Changes proposed in this Pull Request:
This PR copies the javascript from the wpcalypso repo, and into Jetpack, with the hopes of building the blocks from here in Jetpack, where their PHP code lives.
Testing instructions:
extensions/blocks
folder into your local calypso atpackages/jetpack-blocks/src/blocks
packages/jetpack-blocks/src/blocks
runnpm install
npx lerna run build --stream --scope='@automattic/jetpack-blocks' -- -- --output-path /path-to-jetpack/_inc/blocks
Proposed changelog entry for your changes: