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

Extensions: Simplify directories #11971

Merged
merged 3 commits into from
Apr 10, 2019
Merged

Extensions: Simplify directories #11971

merged 3 commits into from
Apr 10, 2019

Conversation

roccotripaldi
Copy link
Member

Fixes a small thread of dignity

Changes proposed in this Pull Request:

  • Removes extensions/setup and puts its content in either the root of the extensions folder, or in extensions/shared
  • Removes extensions/utils and puts its content in extensions/shared

Testing instructions:

  • run yarn build and get no errors
  • verify that extensions in the post editor look ok

Proposed changelog entry for your changes:

  • None

@roccotripaldi roccotripaldi added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack Build labels Apr 8, 2019
@roccotripaldi roccotripaldi requested a review from a team April 8, 2019 20:37
@jetpackbot
Copy link
Collaborator

jetpackbot commented Apr 8, 2019 •
edited
Loading

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: April 29, 2019.
Scheduled code freeze: April 22, 2019

Generated by 🚫 dangerJS against 5f270d1

simison
simison reviewed Apr 9, 2019
View reviewed changes
extensions/shared/README.md Outdated

## registerJetpackPlugin()

This util will only register a Gutenberg plugin if it meets the availability requirements described above.
Copy link
Member

@simison simison Apr 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

We probably need to tell about these methods in more visible /extensions/README.md as part of documentation to get folks started building blocks? Good here for now tho.

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 think i can drop these method descriptions altogether in favor of standard docblocks.

simison
simison reviewed Apr 9, 2019
View reviewed changes
extensions/shared/block-styles.js
*/
export function hasStyleClass( classNames = '' ) {
return classNames.split( ' ' ).some( className => className.startsWith( 'is-style-' ) );
}
Copy link
Member

Choose a reason for hiding this comment

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

Confirmed that these are not used anywhere; I think they used to be in Tiled gallery.

Well spotted!

simison
simison reviewed Apr 9, 2019
View reviewed changes
extensions/shared/register-jetpack-block.js
@@ -6,7 +6,7 @@ import { registerBlockType } from '@wordpress/blocks';
/**
* Internal dependencies
*/
import extensionList from '../setup/index.json';
import extensionList from '../index.json';
Copy link
Member

Choose a reason for hiding this comment

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

Nice move, like this people will find this right away when they start developing blocks.

simison
simison previously approved these changes Apr 9, 2019
View reviewed changes
Copy link
Member

@simison simison left a comment

Choose a reason for hiding this comment

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

Super clear and builds nicely 👍

Tested:

yarn run build-extensions
yarn run test-extensions

kraftbj
kraftbj previously approved these changes Apr 9, 2019
View reviewed changes
Copy link
Contributor

@kraftbj kraftbj left a comment

Choose a reason for hiding this comment

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

Ran the build and spot-check the post editor, so far so good.

I'm manually setting this to allow merging overriding the failed wpcom check. Since this is a big change, please merge when you have the bandwidth to make the wp.com merge happen. I'm forcing a diff to be created, but due to other Jetpack changes that didn't merge yet, we're running into conflicts.

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello roccotripaldi! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D26668-code before merging this PR. Thank you!

@simison
Copy link
Member

simison commented Apr 9, 2019 •
edited
Loading

We were removing some test files from wpcom (they don't need to be synced) so might be that's why there was a conflict

D26665-code & D26669-code

@simison simison added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Apr 9, 2019
@roccotripaldi roccotripaldi dismissed stale reviews from kraftbj and simison via 8fb8572 April 9, 2019 14:01
@roccotripaldi roccotripaldi force-pushed the simplify-extensions-directories branch from 8fb8572 to 89e865e Compare April 9, 2019 14:21
@roccotripaldi
Copy link
Member Author

Rebased and manually updated D26668-code

Everything is still building well! @kraftbj can you re-bless this?

@simison simison force-pushed the simplify-extensions-directories branch from 89e865e to 5f270d1 Compare April 10, 2019 09:13
@simison
Copy link
Member

simison commented Apr 10, 2019 •
edited
Loading

Rebased here, rebased D26668-code and it applies nicely in the sandbox.

@simison simison merged commit 4c63a49 into master Apr 10, 2019
@simison simison deleted the simplify-extensions-directories branch April 10, 2019 11:06
@matticbot matticbot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Apr 10, 2019
@simison simison mentioned this pull request Apr 12, 2019
Closed
3 tasks
@simison simison mentioned this pull request Apr 15, 2019
Closed
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kraftbj kraftbj kraftbj approved these changes

@simison simison simison left review comments

Assignees
No one assigned
Labels
Build [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack Touches WP.com Files
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@roccotripaldi @jetpackbot @matticbot @simison @kraftbj