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

PoC: RRM Gutenberg Blocks #10247

Draft
wants to merge 13 commits into
base: develop
Choose a base branch
from
Draft

PoC: RRM Gutenberg Blocks #10247

wants to merge 13 commits into from

Conversation

nfmohit
Copy link
Collaborator

@nfmohit nfmohit commented Feb 19, 2025

Summary

Addresses issue:

Relevant technical choices

This is a PoC that implements one of the RRM Gutenberg blocks.

PR Author Checklist

  • My code is tested and passes existing unit tests.
  • My code has an appropriate set of unit tests which all pass.
  • My code is backward-compatible with WordPress 5.2 and PHP 7.4.
  • My code follows the WordPress coding standards.
  • My code has proper inline documentation.
  • I have added a QA Brief on the issue linked above.
  • I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

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

Code Reviewer Checklist

  • Run the code.
  • Ensure the acceptance criteria are satisfied.
  • Reassess the implementation with the IB.
  • Ensure no unrelated changes are included.
  • Ensure CI checks pass.
  • Check Storybook where applicable.
  • Ensure there is a QA Brief.
  • Ensure there are no unexpected significant changes to file sizes.

Merge Reviewer Checklist

  • Ensure the PR has the correct target branch.
  • Double-check that the PR is okay to be merged.
  • Ensure the corresponding issue has a ZenHub release assigned.
  • Add a changelog message to the issue.

// We need to import `registerStore` from `./modules/reader-revenue-manager/datastore` rather than `./modules/reader-revenue-manager`
// to avoid pulling in all the other modules imported in the top-level RRM module.
import { registerStore } from '../../../assets/js/modules/reader-revenue-manager/datastore';
import { CORE_MODULES } from '../../../assets/js/googlesitekit/modules/datastore/constants';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Having to reach into assets like this makes me think blocks should have been a subdirectory of assets/js, after all these are still JS assets... And, it would be more closely aligned with the workspace structure.

Maybe one to discuss with the team and/or at the next eng leads sync...

Cc @tofumatt @eugene-manuilov @aaemnnosttv

Copy link
Collaborator

Choose a reason for hiding this comment

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

@techanvil @nfmohit CORE_MODULES and registerStore should be imported from googlesitekit-* package like we do it with googlesitekit-data. If we can't do it right now, then we need to update our assets to be able to do that. Then we will need to add the rrm module script as a dependency to this block script.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@eugene-manuilov, I'm not sure we want to include the full googlesitekit-modules-reader-revenue-manager JS asset on the posts page, as it will contain code and components that we don't want to and indeed can't use due to the differing React versions between our NPM-installed @wordpress/element and that of the external @wordpress-core/element (and possibly some other dependency version clashes).

It could arguably be worthwhile though, either if the commonalities between the RRM block editor asset and the main RRM module grow large enough, or we just figure that browser-side caching of the asset makes it efficient to reuse on the posts page anyway.

If so, we'll need to restructure the asset to avoid unwanted initialisation that is not relevant to the posts page.

If we decide to keep the two separate, it would still be nice to figure out a way to import from googlesitekit-* packages, but we'd probably need to introduce packages and entry points for shared RRM and other relevant module code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for the input here, folks!

As we have a working PoC with the assets not restructured, let's go ahead and use them as they are now. Let's revisit this on our next engineering leads sync. Thanks!

'googlesitekit-modules',
),
'load_contexts' => array( Asset::CONTEXT_ADMIN_POST_EDITOR ),
)
Copy link
Collaborator

@techanvil techanvil Feb 19, 2025

Choose a reason for hiding this comment

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

@nfmohit I suspect we'll need to add execution => defer to this asset, as I found it was needed for the initial asset in my PoC, this was to ensure the store was correctly initialised regardless of whether the Admin Bar is enabled or not.

$assets[] = new Script(
'googlesitekit-reader-revenue-manager-block-editor',
array(
'src' => $base_url . 'js/googlesitekit-reader-revenue-manager-block-editor.js',
'dependencies' => array(
'googlesitekit-data',
'googlesitekit-i18n',
'googlesitekit-modules',
),
'load_contexts' => array( Asset::CONTEXT_ADMIN_POST_EDITOR ),
'execution' => 'defer',
)
);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated, thanks Tom!

@nfmohit nfmohit mentioned this pull request Feb 14, 2025
10 tasks
*/
import { useBlockProps, InspectorControls } from '@wordpress-core/block-editor';
import { ExternalLink, Notice } from '@wordpress-core/components';
import { createInterpolateElement, Fragment } from '@wordpress/element';
Copy link
Collaborator

Choose a reason for hiding this comment

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

@nfmohit, I think these imports should come from @wordpress-core/element to avoid mixing code/components from different versions of React...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, updated, thanks Tom!

<div { ...blockProps }>
<div className="googlesitekit-blocks-reader-revenue-manager">
<EditorButton disabled={ disabled }>
Contribute with Google
Copy link
Collaborator

@techanvil techanvil Feb 20, 2025

Choose a reason for hiding this comment

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

@nfmohit, just to be clear is the expectation that the real button content will always be this text in English?

Asking for clarity and also because the button styling would not accommodate a longer piece of text...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question. In the actual button that renders in the front end, the text is populated by the swg-basic.js library, so it is not translatable, not at least from our end. The swg-basic.js library does populate the button with text using a different language if the site language is different, but since we don't load swg-basic.js in the editor, we can't replicate that. Do you think we should make it translatable for our editor preview?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @nfmohit. Seeing as the actual button will be rendered with text in the site's language, I think we should do our best to translate it in the editor too.

I'd suggest making the text translatable and linking to the list of titles for the relevant button in the translators hint, e.g. here for the Contribute button.

The styling will also need modifying to better handle variable width text.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree, I've updated the button accordingly, thanks Tom!

@techanvil techanvil mentioned this pull request Feb 21, 2025
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants