-
Notifications
You must be signed in to change notification settings - Fork 805
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
My Jetpack: Add initial skeleton #21464
Conversation
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped. Jetpack plugin:
|
Caution: This PR has changes that must be merged to WordPress.com |
"build-production": "echo 'Add your build step to composer.json, please!'", | ||
"build-development": "echo 'Add your build step to composer.json, please!'" |
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.
Looks like you still need to add the build rules.
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.
Co-authored-by: Brad Jorsch <[email protected]>
Co-authored-by: Brad Jorsch <[email protected]>
$build_assets = require_once __DIR__ . '/../build/index.asset.php'; | ||
wp_enqueue_script( 'my_jetpack_main_app', plugin_dir_url( __DIR__ ) . 'build/index.js', $build_assets['dependencies'], $build_assets['version'], true ); |
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.
We should use Assets::register_script()
here. That should automatically handle adding the CSS too once it gets created.
* | ||
* @var boolean | ||
*/ | ||
private static $initialized = false; |
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.
Maybe we could use an action to only init once? It's something I try to keep in mind that was mentioned to me here.
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've seen both patterns and I usually prefer the static property, just because it's much more performant. I can see some advantages that @zinigor points out, but at the end of the day it's all theoretical, not sure we ever used these actions for anything else... Let me think this over... If anyone has strong opinions on that let me know. Not a big deal to me
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 sure we ever used these actions for anything else
It just came to mind since we will be requiring all Jetpack plugins to add this. Exactly what/why someone might hook into that for? 🤷 but it sounds good in theory 😃
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.
ok, changed to the hooks approach, I could argue on the cons of this approach, or why it's not effective (we should add two hooks, one before and one after initialization, and offer a way for people to short circuit the initialization if we wanted this to be really useful).
But I like how explicit it is about what's going on, and easier to test
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.
Great job! 🚢
Great news! One last step: head over to your WordPress.com diff, D68780-code, and commit it. Thank you! |
Just wanted to note b/c of our naming structure, this becomes the |
* master: (28 commits) Prepare for 10.4-a.7 (#21797) Check if maxwidth exists before trying to call .length (#21785) Podcast Player: Increase podcast shown (#21661) licensing: jetpack-js-test-runner is a dev dependency (#21794) Update dependency @mdn/browser-compat-data to v4.0.11 (#21792) Tests: Fix unit tests for wpcom (#21649) SSO: Fix Button Text Alignment on Mobile (#18770) Add/my jetpack skeleton (#21464) Identity Crisis: add unit tests for has_identity_crisis and get_mismatched_urls (#21754) Remove special cases for calling changelogger on packages/changelogger (#21783) eslint-config-target-es: Release 1.0.0! (#21766) Revert "WPCOM no longer forces home_urls to be http" (#21769) WPCOM no longer forces home_urls to be http (#21747) Revert and fix "Revert "Assets: do not use the new method yet (#21760)"" (#21763) eslint-config-target-es: Fix README.md (#21743) codesniffer: Disable CI on PHP 8.1 (#21742) RNA Pricing card component: Fix case where prices before and after match (#21757) Janitorial: update Jetpack version for 10.4-a.6 cycle (#21762) Releases: prepare changelog for 10.4-a.5 release (#21758) Assets: do not use the new method yet (#21760) ...
It doesn't have to if we don't want it to, that's just the default. We could change the name in |
r235273-wpcom |
* master: (28 commits) Prepare for 10.4-a.7 (#21797) Check if maxwidth exists before trying to call .length (#21785) Podcast Player: Increase podcast shown (#21661) licensing: jetpack-js-test-runner is a dev dependency (#21794) Update dependency @mdn/browser-compat-data to v4.0.11 (#21792) Tests: Fix unit tests for wpcom (#21649) SSO: Fix Button Text Alignment on Mobile (#18770) Add/my jetpack skeleton (#21464) Identity Crisis: add unit tests for has_identity_crisis and get_mismatched_urls (#21754) Remove special cases for calling changelogger on packages/changelogger (#21783) eslint-config-target-es: Release 1.0.0! (#21766) Revert "WPCOM no longer forces home_urls to be http" (#21769) WPCOM no longer forces home_urls to be http (#21747) Revert and fix "Revert "Assets: do not use the new method yet (#21760)"" (#21763) eslint-config-target-es: Fix README.md (#21743) codesniffer: Disable CI on PHP 8.1 (#21742) RNA Pricing card component: Fix case where prices before and after match (#21757) Janitorial: update Jetpack version for 10.4-a.6 cycle (#21762) Releases: prepare changelog for 10.4-a.5 release (#21758) Assets: do not use the new method yet (#21760) ...
This PR bootstraps the My Jetpack package, including a React app that will hold the whole My Jetpack page on admin.
For now, it's a very simple skeleton. Header and Footer are there as placeholders as we still need to work on these generic components and shared CSS styles.
Changes proposed in this Pull Request:
Jetpack product discussion
p9dueE-3Ae-p2
Does this pull request change what data or activity we track or use?
No
Testing instructions:
define( 'JETPACK_ENABLE_MY_JETPACK', true );
to yourwp-config.php
My Jetpack
submenu under the Jetpack top-level menuconnection-ui
package