-
Notifications
You must be signed in to change notification settings - Fork 798
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
Custom CSS: copy module to jetpack-mu-wpcom package #37794
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. Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Mu Wpcom plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Wpcomsh 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?
|
15c7565
to
a2ba305
Compare
4b0070e
to
fa1cbbf
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.
This tests well for me on self-hosted, Atomic and Simple.
I may still be misunderstanding the difference between the Simple implementation and what was in Jetpack before (I think I understood what you mentioned before to be that there was a difference between what is loaded between Atomic and Simple?), but I still wonder why the references in mu-plugins/custom-css
and related files can't now point to the corresponding file in the jetpack-mu-wpcom package
(in contrast to pointing to the Jetpack version before), if those are loaded for Simple sites?
I think that would be an aside though (not a blocker).
As for the Phan check failing, I'm baffled there as I'm not seeing anything showing, nor can I run phan for the jetpack-mu-wpcom
package locally. It is getting stuck when attempting to 'analyze'. In the end I was able to begin to get it working by removing all directories except custom-css.php
, which could be a help in beginning to look at some of the Phan issues that aren't related to removing files anyway.
Thanks for your review @coder-karen.
Just so every reader has the same context: Simple sites load the The features in both implementations seem equivalent. But given the scope and timeline of the overarching project, I recommend we keep them separate, at least for the moment. That's why in the
We should probably do that. This would prevent unnecessary code duplication and possibly help us get rid of the PHP warnings triggered by the csstidy library. I believe we need to deploy this PR first, though. I hope this clarifies things a little and doesn't just reiterate what you already knew. |
As for the static analysis failing, I'm glad it's not me missing an obvious log! I haven't been able to locate the culprit in the pipeline or locally either. This conversation (p1718210071497239-slack-CBG1CP4EN) points to the |
6bfe5bc
to
db3a9f7
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.
We should probably do that. This would prevent unnecessary code duplication and possibly help us get rid of the https://github.com/Automattic/vulcan/issues/362. I believe we need to deploy this PR first, though.
Yes that's what I was initially thinking of in terms of the duplication :) But yes, that'd be a next step anyway.
I've left a few comments as I've looked at this some more.
One of the main things I've noticed is that we should be building the CSS and JS files (.min, .map) using Webpack, as with all other packages. We don't include anything other than the .css and .js files in the monorepo, but only in production after building.
Let me know if you'd like me to help with this :)
Edit - actually the JS files are fine as they are (copied over directly). But the jetpack-mu-wpcom webpack.config file would ideally need to be updated to build the custom-css and csstidy CSS.
projects/packages/jetpack-mu-wpcom/src/features/custom-css/custom-css.php
Outdated
Show resolved
Hide resolved
|
||
wp_register_script( | ||
'jetpack-customizer-css-preview', | ||
Assets::get_file_url_for_environment( |
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.
As with the other similar comment, it might be worth rewriting this in relation to get_file_url_for_environment
.
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.
Hmm that other comment didn't get added - re-added now: https://github.com/Automattic/jetpack/pull/37794/files#r1637844413
projects/packages/jetpack-mu-wpcom/src/features/custom-css/custom-css.php
Show resolved
Hide resolved
wp_register_style( 'jetpack-customizer-css', plugins_url( 'custom-css/css/customizer-control.css', __FILE__ ), array(), '20140728' ); | ||
wp_register_script( 'jetpack-codemirror', plugins_url( 'custom-css/js/codemirror.min.js', __FILE__ ), array(), '3.16', true ); | ||
|
||
$src = Assets::get_file_url_for_environment( |
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.
Seems the suggestion I added here didn't post initially so re-adding it. With get_file_url_for_environment
, there was a suggestion to avoid using this if possible in packages. Slack ref: p1715614401962229-slack-CBG1CP4EN
Here is how I changed it in Calypsoify: https://github.com/Automattic/jetpack/pull/37351/files
b9250ed
to
9abc1d2
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.
I know the linked issue is listed as 'in progress' so this might not be open for re-review just yet, but since I was taking another look I thought I'd drop some more comments in case it could be helpful.
I notice that the CSSTidy built files have also been copied over to this PR - CSSTidy was also being built only in production before. In the monorepo it is just .css
and .php
files at the moment.
In production, we weren't including .map
files (they should only be relevant for development builds, rather than production builds).
Also the currently committed minified versions are not actually minified (I'm guessing, as with the other built files in the csstidy
folder, due to having being built with a development build locally, then copied over from the Jetpack plugin modules folder?)
With the Assets changes, I understand the Assets workaround here now is to use the non-minified versions. Another alternative would include building to the same directory the non minified files are in. Garage might also have insight about why using the build path wasn't working (if that was still the issue from before).
Also I'm curious about why the Codemirror files were removed.
Apologies if this was all still in progress :D
e1051cb
to
6b5ca9b
Compare
1f3f20e
to
82a9f6f
Compare
Seeing some fatals on sites that appears to be related to this change. It seems to be an issue if a site/theme is using their own implementation of lessc. Is there anyway to disable the Jetpack module entirely on AT sites so that it does not load? Deactivating the Custom CSS module in BRC did not seem to help. Here is a stack trace from a user report where their theme is loading it. I have another request from another user that wants to disable lessc from Jetpack entirely as well. 8421952-zd-a8c
|
Fixes https://github.com/Automattic/vulcan/issues/361
Proposed changes:
The Custom CSS feature in Jetpack will soon be deprecated. To keep the feature working for WoA sites, we're copying the module to the
jetpack-mu-wpcom
package (note that this PR doesn't remove the Custom CSS module from Jetpack yet).Simple sites load their own implementation of the Custom CSS feature and should not be impacted by this PR.
The file diff is a bit intimidating but we've mostly copied files. In more detail, here's what this PR does:
jetpack-mu-wpcom
packagejetpack-mu-wpcom
to build the custom CSS assetsAssets::register_script
wp_admin_enqueue_scripts
hook to prevent errors when running testsscssphp
package injetpack-mu-wpcom
class-jetpack-mu-wpcom.php
for WoA sites onlyjetpack-mu-wpcom
package. This prevents PHP errors no matter the loading order of Jetpack and WordPress Features plugins.Other information:
Jetpack product discussion
pfwV0U-4M-p2
Does this pull request change what data or activity we track or use?
No.
Testing instructions:
Let's do some regression testing.
Simple sites
bin/jetpack-downloader test jetpack update/copy-custom-css-to-mu-wpcom
Self-hosted sites
/wp-admin/admin.php?page=jetpack_modules
, activate the Custom CSS moduleWoA sites
/wp-admin/admin.php?page=jetpack_modules
, activate the Custom CSS moduleOptionally, ssh to your WoA site and delete both the
custom-css.php
file and thecustom-css
folder in~/htdocs/wp-content/plugins/jetpack-dev/modules
. Ensure the Custom CSS feature works as before.