jetpack icon indicating copy to clipboard operation
jetpack copied to clipboard

Subscription Site: Add "preview and edit" link to the Subscribe block placement

Open pkuliga opened this issue 11 months ago • 2 comments

Part of https://github.com/Automattic/jetpack/issues/36021

Proposed changes:

It:

  • adds the "preview and edit" link to the Subscribe block placement settings
  • replaces the existing "Preview and edit the pop-up" link with the "preview and edit" to keep the consistency
Screenshot 2024-03-06 at 15 20 24

Other information:

  • [ ] Have you written new tests for your changes, if applicable?
  • [ ] Have you checked the E2E test CI results, and verified that your changes do not break them?
  • [ ] Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

  • enable the Subscription Site feature by adding the following line to your 0-sandbox.php file:
add_filter( 'jetpack_subscription_site_enabled', '__return_true' );
  • enable the Jetpack Newsletter Module
  • with the block theme enabled, go to Jetpack Newsletter Settings and make sure the links are there and work as expected
  • with the classic theme enabled, go to Jetpack Newsletter Settings and make sure "preview and edit" links are not visible

pkuliga avatar Mar 06 '24 14:03 pkuliga

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • :white_check_mark: Include a description of your PR changes.
  • :white_check_mark: Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • :white_check_mark: Add testing instructions.
  • :white_check_mark: Specify whether this PR includes any changes to data or privacy.
  • :white_check_mark: Add changelog entries to affected projects

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 :robot:


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. Then, add the "[Status] Needs Team Review" label and ask someone from your team review the code. Once reviewed, it can then be merged. If you need an extra review from someone familiar with the codebase, you can update the labels from "[Status] Needs Team Review" to "[Status] Needs Review", and in that case Jetpack Approvers will do a final review of your PR.


Jetpack plugin:

The Jetpack plugin has different release cadences depending on the platform:

  • WordPress.com Simple releases happen daily.
  • WoA releases happen weekly.
  • Releases to self-hosted sites happen monthly. The next release is scheduled for April 2, 2024 (scheduled code freeze on April 1, 2024).

If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack.

github-actions[bot] avatar Mar 06 '24 14:03 github-actions[bot]

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

  • To test on WoA, go to the Plugins menu on a WordPress.com Simple site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin, and enable the update/subscription-site-settings-template-link branch.

  • To test on Simple, run the following command on your sandbox:

    bin/jetpack-downloader test jetpack update/subscription-site-settings-template-link
    

Interested in more tips and information?

  • In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
  • Read more about our development workflow here: PCYsg-eg0-p2
  • Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

github-actions[bot] avatar Mar 06 '24 14:03 github-actions[bot]

Nitpicking on copy;

  • other toggle copy ends in ., other doesn't. Which way should it be?
  • Starting in lower capital feels a bit weird for translators; it might be a non-issue. Should we perhaps ditch () and just make the whole sentence "full" proper sentence?

[ ] Toggle label. Preview and edit.

@crisbusquets for direction.

simison avatar Mar 06 '24 19:03 simison

  • other toggle copy ends in ., other doesn't. Which way should it be?
  • Starting in lower capital feels a bit weird for translators; it might be a non-issue. Should we perhaps ditch () and just make the whole sentence "full" proper sentence?

[ ] Toggle label. Preview and edit.

Fixed, thanks for spotting!

pkuliga avatar Mar 11 '24 11:03 pkuliga

Hi @pkuliga !

I tested it and it works as expected.

But based on what I shared on the i3 post, can we release a version with this structure?:

Screenshot 2024-03-11 at 13 25 27

Instead of:

Screenshot 2024-03-11 at 13 24 38

crisbusquets avatar Mar 11 '24 12:03 crisbusquets

@crisbusquets adding a main toggle introduces too much complexity. Let's do it when we have more toggles.

pkuliga avatar Mar 11 '24 12:03 pkuliga

@crisbusquets adding a main toggle introduces too much complexity. Let's do it when we have more toggles.

But I wasn't referring to the main toggle for Newsletters (we're still discussing it in the P2).

The screenshots I shared in my previous comment were almost the same, the only difference is the structure for the toggles. It separates them into two, instead of one: one for subscriptions (modal, and automatic insertion) and one for comments-related subscriptions.

Now I'm not sure if we're talking about the same main toggle, tho 😅

crisbusquets avatar Mar 11 '24 13:03 crisbusquets

@crisbusquets where should be the "Let visitors subscribe to new posts and comments via email" toggle in the new structure? AFAIK, this toggle works now as a "main toggle" to enable/disable the Newsletter module. Do you want to remove it?

pkuliga avatar Mar 12 '24 09:03 pkuliga

@crisbusquets where should be the "Let visitors subscribe to new posts and comments via email" toggle in the new structure? AFAIK, this toggle works now as a "main toggle" to enable/disable the Newsletter module. Do you want to remove it?

Just adding a comment here to note that we discussed this in our weekly call today.

Let's ship this version, and improve copy so it's more concise. Here's the new issue:

https://github.com/Automattic/jetpack/issues/36342

crisbusquets avatar Mar 12 '24 11:03 crisbusquets