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

Add post-level RRM settings #9962

Open
5 tasks done
nfmohit opened this issue Jan 2, 2025 · 16 comments
Open
5 tasks done

Add post-level RRM settings #9962

nfmohit opened this issue Jan 2, 2025 · 16 comments
Labels
Module: RRM Reader Revenue Manager module related issues P0 High priority Team M Issues for Squad 2 Type: Enhancement Improvement of an existing feature

Comments

@nfmohit
Copy link
Collaborator

nfmohit commented Jan 2, 2025

Feature Description

As part of RRM Phase 2, the WordPress post editor will allow users to override the snippet configuration at the post level. This issue should implement the fields to allow the said configuration.

Example mock:
Image

The select field having a label of “Product ID Override”* will reflect and change the value of the googlesitekit_rrm_{Publication ID}:productID post meta setting with the following options:

  1. No change”*; Default; Value: Empty string.
  2. Off”*; Value: none.
  3. Open access”*; Value: openaccess.
  4. Any available product IDs for the connected publication, obtainable using the new productIDs module setting.

* The final version of the labels should be checked and confirmed with Sigal and Mariya.

As this will be rendered within the WordPress post editor, the new googlesitekit-reader-revenue-manager-post-editor.js JavaScript entry point should be used here.

The PluginDocumentSettingPanel component from @wordpress/editor should be used to compose a setting panel for Site Kit. It should use relevant components from @wordpress/components to implement the necessary setting field.

A PoC has already implemented this largely which can be used as a starting point.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • If the rrmModuleV2 feature flag is enabled, the Reader Revenue Manager module is connected, and the current user has access to the Reader Revenue Manager module:
    • A new panel should be added to the document settings sidebar of the WordPress block editor. It should have a title of "Google Site Kit" and a Google icon (logo-g.svg).
    • It should include a heading of "Reader Revenue Manager".
    • It should include a select field with a label of "Decide how site visitors should access this post (if they will see CTAs by Reader Revenue Manager, which you activated via Site Kit):" (in normal case - WordPress converts these to appear in uppercase).
    • It should have the following options:
      • "Keep the default selection"
      • "Exclude from Reader Revenue Manager"
      • "Use "open access""
      • "Use "$PRODUCTID"" (this should be added for each available product ID)
    • It should have a helper text only if anything but the first default option has been selected, as follows:
      • "This will override any other settings you might have applied in Site Kit.".
    • When the post is updated in the block editor, the selection should be saved using the added post meta setting.
    • The styling for the added elements should follow the WordPress design language and be in line with other parts of the block editor.
    • The added setting should be backwards-compatible down to WP 5.4.

Implementation Brief

Please refer to the updated PoC.

Webpack configuration

  • Update webpack/common.js:
    • The block editor panel will depend on the external versions of @wordpress/components, @wordpress/edit-post and @wordpress/plugins. These should be added to corePackages.
    • Additionally, googlesitekit-api, googlesitekit-data and googlesitekit-modules should be added to gutenbergExternals.
  • Update webpack/adminCss.config.js:
    • We'll need to add a new ./assets/sass/block-editor.scss SASS file for block editor styling. This should be added as a new entry with the name googlesitekit-block-editor-css.

Block editor SCSS file

  • Create a new file at assets/sass/block-editor.scss, and another at assets/sass/components/reader-revenue-manager/_googlesitekit-rrm-block-editor.scss which block-editor.scss can include.
    • Add styling as needed to meet the AC - notably, the point about ensuring the select label is lowercase. It will also be necessary to ensure the select label's text is not truncated.

Add/update Assets

  • Update includes/Modules/Reader_Revenue_Manager.php:
    • Add a new Stylesheet asset for the googlesitekit-block-editor-css.css CSS file. Ensure load_contexts is Asset::CONTEXT_ADMIN_POST_EDITOR for the asset.
    • Update the googlesitekit-reader-revenue-manager-block-editor asset:
      • Include the googlesitekit-data, googlesitekit-i18n and googlesitekit-modules dependencies.
      • Ensure execution is specified as defer - this is necessary to ensure that the store setup won't clash with the Admin Bar when that's loaded.

Panel implementation

  • Update assets/js/googlesitekit-reader-revenue-manager-block-editor.js:
    • Follow the updated PoC implementation.
      • Note the following:
        • When importing from @wordpress-core/components we are specifying the modules to be externals. We don't have these installed locally via NPM so it's necessary to disable the import/no-unresolved ESLint rule to avoid linting errors.
        • Due to googlesitekit-data using the local version of @wordpress/element (via the local @wordpress/data), we can't use useSelect(), as this would try to use a different version of React compared to the external version that Gutenberg is using. We may introduce a version of googlesitekit-data which uses the external @wordpress-core/data but, if we do, this will be introduced at a later date as we need to spend some time figuring out the technical approach and implications. For the time being we simply avoid using useSelect() in the RRM panel, fortunately it's quite a simple piece of UI code and this isn't a significant hindrance.

Test Coverage

  • Add tests where practical for new components added for the block editor.
  • Update any failing tests.

QA Brief

  1. In the publisher center, ensure that the publication has at least 1 productID available. It can be setup through paywall or contribution by "Add Plan" CTA. Refer to the screenshot below:
Image
  1. Install and activate Site Kit. Connect to RRM module and ensure that rrmModule and rrmModuleV2 flags are enabled.

  2. Go to Posts > Add new and ensure that Selection panel is visible in the block editor as shown in screenshot below.

Image
  1. Change the value in dropdown and it should show the helper text as mentioned in AC.

  2. Save the value and it should save the value which should persist after the post is saved and refreshed.

Changelog entry

  • Add a panel to the WordPress post editor to users to override the Reader Revenue Manager snippet configuration at the post level.
@nfmohit nfmohit self-assigned this Jan 2, 2025
@nfmohit nfmohit added P0 High priority Type: Enhancement Improvement of an existing feature Team M Issues for Squad 2 labels Jan 2, 2025
@nfmohit
Copy link
Collaborator Author

nfmohit commented Jan 6, 2025

@aaemnnosttv Going back to our discussion regarding the settings in the block editor, we have two options:

  1. Separate plugin panel: This is supported in WordPress 5.2 and above. The plugin is pinned to the header by default but can be unpinned by the user. When unpinned, it can be accessed using the "more" menu.

Image

  1. Within the document sidebar: This is not supported in WordPress 5.2 but is supported on 5.3 and above (according to when I last checked).

Image

In my PoC, I have added both options. IIRC, you were inclined towards option 1 as it has wider support. Just to confirm, should we go with option 1 and abandon option 2?

Once I have some feedback on this, I'll draft the ACs accordingly and seek advice from Sigal and Mariya on the copy.

Thank you!

@nfmohit
Copy link
Collaborator Author

nfmohit commented Jan 9, 2025

Update: Upon discussion on the above with @aaemnnosttv internally, we found ourselves inclined towards option 1 as it has wider support, and it also adds a dedicated place for Site Kit in the block editor for potential future field additions. I'll draft the ACs and seek assistance from Sigal regarding UX if required.

@nfmohit nfmohit assigned aaemnnosttv and unassigned aaemnnosttv and nfmohit Jan 9, 2025
@nfmohit nfmohit added the Module: RRM Reader Revenue Manager module related issues label Jan 18, 2025
@ivonac4 ivonac4 added the Next Up Issues to prioritize for definition label Jan 21, 2025
@nfmohit
Copy link
Collaborator Author

nfmohit commented Jan 22, 2025

@marrrmarrr I've co-assigned this to you in hopes of receiving feedback on the copy of the setting field in the ACs. These were drafted before your brilliant suggestions in #9961. I hope that is okay, thank you! 🙏

@marrrmarrr
Copy link
Collaborator

I'd suggest keeping consistent with the copy in the Categories page and adjusting it so it applies to posts, that way hopefully users understand the parallels between the two and that they're part of the same feature:

Top heading: Decide how site visitors should access this post (if they will see CTAs by Reader Revenue Manager, which you activated via Site Kit):

  • keep the default selection from Site Kit settings
  • exclude this post from Reader Revenue Manager
  • use "open access" for this post
  • use "$PRODUCTID" for this post

Below the dropdown field: If you select a different option than the default, it will override any other settings you might have applied in Site Kit or in the Categories section.

@nfmohit do you think these options would fit in the current design, or the copy is too long for the sidebar?

@nfmohit
Copy link
Collaborator Author

nfmohit commented Feb 5, 2025

Thank you for the excellent suggestions, @marrrmarrr!

I've quickly used your suggested copy in my PoC and would like to demonstrate how it looks:

Image

They do fit, but I'm not sure they look great as the sidebar real-estate is quite shrinked. Let me know what you think, thank you!

@ivonac4
Copy link
Collaborator

ivonac4 commented Feb 7, 2025

Hi @marrrmarrr , kind reminder for Nahid's question above. We are looking to get this ticket in the sprint as soon as possible.

@nfmohit
Copy link
Collaborator Author

nfmohit commented Feb 12, 2025

In the essence of timings, we're moving this forward as we should be able to update the copy at a later time, thank you Ivona and Mariya!

@techanvil techanvil self-assigned this Feb 12, 2025
@marrrmarrr
Copy link
Collaborator

Thanks @nfmohit for mocking this up and apologies for missing this earlier in the week. A couple of suggestions:

  1. Would it be possible to remove the all caps formatting of the initial explanation ("Decide how site visitors should access..."), so it's a bit less conspicuous?

  2. It seems that the individual lines in the dropdown are a bit long, so I'd suggest shortening the options:

  • "keep the default selection from Site Kit settings" --> "keep the default selection"
  • "exclude this post from Reader Revenue Manager" --> "exclude from Reader Revenue Manager"
  • "use "open access" for this post" --> "use "open access"
  • "use "$PRODUCTID" for this post" --> "use "$PRODUCTID"

@nfmohit
Copy link
Collaborator Author

nfmohit commented Feb 12, 2025

Thanks so much for the suggestions, @marrrmarrr! I have updated the ACs accordingly.

CC: @techanvil

@nfmohit
Copy link
Collaborator Author

nfmohit commented Feb 12, 2025

@aaemnnosttv FYI, the minimum WP version for this feature had to be raised to 5.4, as not all minor versions of 5.3 supports the addition of the panel. Thanks!

@nfmohit
Copy link
Collaborator Author

nfmohit commented Feb 15, 2025

IB ✅

Excellent IB and PoC, thank you @techanvil !

@nfmohit nfmohit removed their assignment Feb 15, 2025
@ankitrox ankitrox self-assigned this Feb 17, 2025
@nfmohit
Copy link
Collaborator Author

nfmohit commented Feb 17, 2025

@ankitrox It might be worth waiting for #10046 to be merged as that already encapsulates a lot of the foundational pieces of work required for this issue. Besides, that also changes a bunch of files that we're planning to use here.

@techanvil What do you think?

CC: @ivonac4

@techanvil
Copy link
Collaborator

Thanks @nfmohit, as discussed on Slack this is a good observation and it would indeed be waiting for the PR for #10046 to be merged before proceeding with this one.

@ivonac4 ivonac4 removed the Next Up Issues to prioritize for definition label Feb 17, 2025
@ankitrox ankitrox removed their assignment Feb 21, 2025
@techanvil techanvil assigned techanvil and ankitrox and unassigned techanvil Feb 21, 2025
@ankitrox ankitrox assigned techanvil and unassigned ankitrox Feb 24, 2025
@techanvil techanvil assigned ankitrox and unassigned techanvil Feb 24, 2025
@ankitrox ankitrox assigned techanvil and unassigned ankitrox Feb 24, 2025
@techanvil techanvil assigned ankitrox and unassigned techanvil Feb 24, 2025
@techanvil
Copy link
Collaborator

Hey @ankitrox, I've merged this issue's PR, however please can you update the screenshot under point 3 of the QAB to reflect the updated styling to save any confusion.

@ankitrox ankitrox removed their assignment Feb 25, 2025
@ankitrox
Copy link
Collaborator

Thanks @techanvil . I've updated the screenshot and unassigned myself.

@techanvil
Copy link
Collaborator

That's great, thanks @ankitrox.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module: RRM Reader Revenue Manager module related issues P0 High priority Team M Issues for Squad 2 Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

6 participants