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.

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

Mailchimp: only load CSS when needed. #11128

Merged
merged 3 commits into from Jan 29, 2019
Merged

Mailchimp: only load CSS when needed. #11128

merged 3 commits into from Jan 29, 2019

Conversation

jeherve
Copy link
Member

@jeherve jeherve commented Jan 11, 2019

Changes proposed in this Pull Request:

This PR introduces 3 changes:

  1. Only enqueue the shortcode / block's CSS file when we need it. It was previously enqueued on all pages, because it was declared as style when registering the block. We don't need to declare it there since the style is actually enqueued manually in parse_shortcode.
  2. Fix typos in 2 files when checking if a file has already been enqueued. wp_style_is only accepts 'enqueued', 'registered', 'queue', 'to_do', and 'done'.
  3. Add the shortcode / block's CSS file to the concatenated jetpack.css file that is automatically generated by Jetpack. This way we don't ever need to load the CSS file separately anymore.

Testing instructions:

  • Checkout the branch
  • Add add_filter( 'jetpack_implode_frontend_css', '__return_false', 99 ); to your site.
  • Load any page. You should not see the jetpack-email-subscribe CSS stylesheet loaded anywhere.
  • Load a page where you have added the [jetpack-email-subscribe] shortcode. You should see the stylesheet loaded there.
  • Add define( 'JETPACK_BETA_BLOCKS', true ); to your site's wp-config.php file.
  • Load the Gutenberg editor.
  • Try to add the Mailchimp block and make sure it looks good.
  • Load the page where the block is displayed and make sure the page includes the stylesheet.
  • Remove the filter I mentioned in step 1.
  • Run yarn build
  • Reload the pages with the shortcode and the block. You should not see the stylesheet being loaded anymore, but instead you should see some jetpack-email-subscribe styles inside the jetpack.css file that is enqueued.

Proposed changelog entry for your changes:

  • Mailchimp: only load CSS when needed.

@jeherve jeherve added [Type] Bug When a feature is broken and / or not performing as intended [Feature] Shortcodes / Embeds [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Block] Mailchimp labels Jan 11, 2019
@jeherve jeherve added this to the 7.0 milestone Jan 11, 2019
@jeherve jeherve self-assigned this Jan 11, 2019
@jeherve jeherve requested a review from artpi January 11, 2019 10:54
@jeherve jeherve requested a review from a team as a code owner January 11, 2019 10:54
@matticbot
Copy link
Contributor

D23075-code. (newly created revision)

@jetpackbot
Copy link

jetpackbot commented Jan 11, 2019

Warnings
⚠️

The PR is missing at least one [Status] label. Suggestions: [Status] In Progress, [Status] Needs Review

This is automated check which relies on PULL_REQUEST_TEMPLATE.We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against cf3cd6e

@jeherve
Copy link
Member Author

jeherve commented Jan 11, 2019

For others who may want to get rid of the stylesheet today, before that patch ships, you can use the following snippet:

function jetpackcom_dequeue_mailchimp_style() {
	wp_dequeue_style( 'jetpack-email-subscribe' );
	wp_deregister_style( 'jetpack-email-subscribe' );
}
add_action( 'wp_enqueue_scripts', 'jetpackcom_dequeue_mailchimp_style' );

Copy link
Member

@mdawaffe mdawaffe left a comment

Choose a reason for hiding this comment

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

I had some trouble replicating the issue in master. I think it was because I had not yet connected a Mailchimp account.

LGTM, though.

mdawaffe
mdawaffe previously approved these changes Jan 11, 2019
Copy link
Member

@mdawaffe mdawaffe left a comment

Choose a reason for hiding this comment

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

I had some trouble reproducing the problem in master. I think it was because I had not yet connected a Mailchimp account.

Got it working and looks good to me.

@jeherve jeherve added [Pri] High [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Jan 15, 2019
- `wp_style_is` only accepts 'enqueued', 'registered', 'queue', 'to_do', and 'done'. Switch to using
enqueued instead of enqueue.
- The style was previously declared when the block was registered. It caused it to be enqueued on all
pages of the site, even when the block or the shortcode are not in use. Since we enqueue the file
manually ourselves inside parse_shortcode, we don't need to enqueue it anywhere else.
`wp_style_is` only accepts 'enqueued', 'registered', 'queue', 'to_do', and 'done'. Switch to using
enqueued instead of enqueue.
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Jan 21, 2019
@matticbot
Copy link
Contributor

D23350-code. (newly created revision)

@jeherve jeherve added the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label Jan 25, 2019
Copy link
Member

@mdawaffe mdawaffe left a comment

Choose a reason for hiding this comment

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

Managed to repro this time. Still looks good to me.

@jeherve jeherve removed the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label Jan 29, 2019
@jeherve jeherve merged commit 51a5be7 into master Jan 29, 2019
@jeherve jeherve deleted the fix/enqueues-css-block branch January 29, 2019 07:34
jeherve added a commit that referenced this pull request Jan 29, 2019
@ockham
Copy link
Contributor

ockham commented Feb 4, 2019

The phab diffs linked from this PR (D23075-code and D23350-code) seem to have only updated Simple Payments but not Mailchimp...

@ockham
Copy link
Contributor

ockham commented Feb 4, 2019

Probably since Fusion wasn't syncing Mailchimp files when this PR was created.
I've just filed D23897-code to sync manually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Mailchimp [Feature] Shortcodes / Embeds [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Pri] High Touches WP.com Files [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants