-
Notifications
You must be signed in to change notification settings - Fork 799
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
Transfer Verbum app into into jetpack-mu-wpcom #34993
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 appearing at the bottom of this PR are passing or skipped. Mu Wpcom plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
h: [ 'preact', 'h' ], | ||
Fragment: [ 'preact', 'Fragment' ], | ||
} ), | ||
new CopyPlugin( { |
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.
Copies Verbum php files into build/verbum-comments
folder.
@@ -34,7 +58,10 @@ module.exports = { | |||
} ), | |||
|
|||
// Handle CSS. | |||
jetpackConfig.CssRule(), | |||
jetpackConfig.CssRule( { | |||
extensions: [ 'css', 'sass', 'scss' ], |
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.
Handles scss conversion to css
plugins: [ | ||
...jetpackConfig.StandardPlugins( { | ||
DependencyExtractionPlugin: { injectPolyfill: true }, | ||
MiniCssExtractPlugin: { filename: '[name]/[name].css' }, |
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.
Outputs CSS in the same build directory as the entry.
For Verbum, css will output in build/verbum-comments
.
{ | ||
"plugins": [ | ||
[ | ||
"@babel/plugin-transform-react-jsx", |
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.
Converts JSX to react function calls.
projects/packages/jetpack-mu-wpcom/src/features/verbum-comments/vite.config.ts
Outdated
Show resolved
Hide resolved
projects/packages/jetpack-mu-wpcom/src/features/verbum-comments/src/utils.ts
Outdated
Show resolved
Hide resolved
...packages/jetpack-mu-wpcom/src/features/verbum-comments/src/components/editor-placeholder.tsx
Show resolved
Hide resolved
* Load Verbum Comments. | ||
*/ | ||
public static function load_verbum_comments() { | ||
if ( class_exists( 'Verbum_Comments' ) ) { |
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.
class_exists condition will be removed when this is finalized, merged into WPCOM, removed from mu-plugins/verbum and we're ready to transfer over to Verbum_Comments inside of jetpack-mu-wpcom plugin.
*/ | ||
export function isFastConnection() { | ||
// Hardcoding the size of the bundle. | ||
const bytes = 30000; |
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.
prod build is 232kb for me and 51.6kb are transferred in the networks tab.
how do we plan to manage this value?
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 don't have a solution for calculating and setting this value as of now.
I removed my initial solution since it wouldn't be efficient enough. However, we still need to figure out a way to either calculate this value or another method to determine what a "fast connection" would be.
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.
51.6kb are transferred in the networks tab.
I see 51.3kb transferred if I use the dev build and 29.5kb with the production build.
Can you check again?
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.
Hey @fushar can you please smoke test the |
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.
@fullofcaffeine, could you take a look at this? I honestly don't know how to fully test the |
@heavyweight - I pushed a change to fix the logic when you click outside of the Subscription modal |
Thanks for the ping! I tested on Simple and the feature looks broken now: I don't have the bandwidth to investigate this, could you help? All information should be available in this PR, the only PR that introduced the feature: |
Thanks for taking the time. |
Checked and works great! |
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.
LGTM
* @return boolean | ||
*/ | ||
private function should_disable_comment_experience( $blog_id ) { | ||
require_once WP_CONTENT_DIR . '/lib/wpforteams/functions.php'; |
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.
Thought: this is going to fail on Atomic sites, as this code only exists on WordPress.com Simple. Or did we already verify that it will work?
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.
Proposed changes:
Moves Verbum Comments project into Jetpack mu wpcom plugin. It also has some glue code to handle the transition between the deployed and the upcoming versions.
Other information:
Does this pull request change what data or activity we track or use?
Not at all.
Testing instructions:
projects/packages/jetpack-mu-wpcom
pnpm run build-js
to build Verbum (development) or Runnpm run build-production-js
(production)jetpack rsync jetpack mu-wpcom-plugin
and select your sandbox as the destination (full command examplejetpack rsync mu-wpcom-plugin user@hostname:~/public_html/wp-content/mu-plugins/jetpack-mu-wpcom-plugin/production
define( 'JETPACK_AUTOLOAD_DEV', true );
in amu-plugin/0-sandbox.php
.comment-experience-loader.php
and makeload_verbum_comments
return early.verbum-comment.js
bundle in Network tab.