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

Create new Ads module codebase structure in assets/js/modules #8225

Closed
21 of 22 tasks
10upsimon opened this issue Feb 5, 2024 · 9 comments
Closed
21 of 22 tasks

Create new Ads module codebase structure in assets/js/modules #8225

10upsimon opened this issue Feb 5, 2024 · 9 comments
Labels
javascript Pull requests that update Javascript code Module: Ads Google Ads module related issues P0 High priority Type: Feature New feature

Comments

@10upsimon
Copy link
Collaborator

10upsimon commented Feb 5, 2024

Feature Description

A new standalone module is required to be introduced to the Google Site Kit plugin. This module should follow the design patterns of existing modules that can currently be found in the assets/js/modules directory within the existing code base. Said new module should be named ads, and should reside within a newly created directory at assets/js/modules/ads. Where applicable, the adsModule feature flag should be used to gate module functionality and presence/visibility. See issue #8197 for more context.

Initially, during this phase, this module will not have any dashboard facing views.

See the applicable section(s) within the Ads Module design document: https://docs.google.com/document/d/1APuSv95bf62uhzlaFlW6jPrzPKy1avpRYd9W1MSAAJo/edit?resourcekey=0-UuynlcUz9CoubgldR6Z5sg#heading=h.9pk2w04qgbdj


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

Acceptance criteria

  • A new Ads module should be present within the Site Kit application, gated by way of the recently added adsModule feature flag see issue Add adsModule Feature Flag #8197.
  • Implementation of a module entrypoint file (index.js) at assets/js/modules/ads/index.js is to be implemented, and is to follow necessary logical patterns relative to module data store(s), settings views/components etc. This module should follow the same pattern as other simpler modules, such as the search-console module, which can be used as a guideline for basic base implementation.
  • An applicable ads module store should be associated with the Ads module. Ads store creation is a separate GH issue, see (TBA).
  • An applicable component for settings editing within the plugin settings area of said ads module should be defined, i.e a SettingsEditComponent, and should follow the design patterns of existing modules.
  • An applicable component for module settings viewing within the plugin settings area of said ads module should be defined, i.e SettingsViewComponent, and should follow the design patterns of existing modules.
  • An applicable form component required to edit settings should be defined, ie SettingsFormComponent, and should follow the design patterns of existing modules.
  • An applicable Ads icon should be created (see Figma) for use within the module scaffolding, as and where needed.
  • Module assets should be defined in the applicable Ads module class by way of a setup_assets() method, for reference see other areas of the codebase, such as:

protected function setup_assets() {
$base_url = $this->context->url( 'dist/assets/' );
$dependencies = array(
'googlesitekit-api',
'googlesitekit-data',
'googlesitekit-datastore-site',
'googlesitekit-modules',
'googlesitekit-vendor',
'googlesitekit-components',
);
$analytics_exists = apply_filters( 'googlesitekit_module_exists', false, 'analytics' );
// Note that the Tag Manager bundle will make use of the Analytics bundle if it's available,
// but can also function without it, hence the conditional include of the Analytics bundle here.
if ( $analytics_exists ) {
$dependencies[] = 'googlesitekit-modules-analytics';
}
return array(
new Script(
'googlesitekit-modules-tagmanager',
array(
'src' => $base_url . 'js/googlesitekit-modules-tagmanager.js',
'dependencies' => $dependencies,
)
),
);
}

  • The base Ads module PHP class should be updated (if not already) to implement the Module_With_Assets interface
  • The base Ads module PHP class should be updated (if not already) to use the Module_With_Assets_Trait trait
  • A module entry point file at assets/js/googlesitekit-modules-ads.js should be created, following the same pattern as existing modules
    • Said entry point file should be included in the Webpack config file at ./webpack.config.js via the module config file at ./webpack/modules.config.js

Implementation Brief

  • Add assets/js/modules/ads/datastore/constants
    • Include a new datastore constant MODULES_ADS
  • Add assets/js/modules/ads/index.js:
    • Use assets/js/modules/search-console/index.js as a starting point to borrow the core logic from
    • Register Ads module, wrapped under adsModule feature flag.
    • You can extract the icon from figma
    • You can omit widget registration logic, as module does not include a widget area/widgets
  • Add assets/js/modules/ads/datastore/index.js
    • Use assets/js/modules/search-console/datastore/index.js as starting point
    • For store use initially a placeholder object, with properties like initialState: {}, actions: {} etc. These will be handled/added in other issue
  • Add assets/js/modules/ads/components/settings/SettingsEdit.js
    • Use assets/js/modules/search-console/components/settings/SettingsEdit.js as starting point
    • Keep hasSearchConsoleAccess, isDoingSubmitChanges and viewComponent component rendering logic
    • Render the main div wrapper with class googlesitekit-setup-module googlesitekit-setup-module--ads
  • Add assets/js/modules/ads/components/settings/SettingsForm.js
    • Use assets/js/modules/search-console/components/settings/SettingsForm.js as a starting point to get the basic logic from
    • Use the getModule selector on CORE_MODULES to retrieve ads module
    • Render main div wrapper with class googlesitekit-ads-settings-fields
    • Include the StoreErrorNotices component
  • Update Google\Site_Kit\Modules\Ads class:
    • Follow the guidelines from AC, regarding the setup_assets method, interfaces, etc
  • Add assets/js/googlesitekit-modules-ads.js file
    • Adapt it from assets/js/googlesitekit-modules-search-console.js
  • Update webpack/modules.config.js:
    • Add googlesitekit-modules-ads to the list pointing to the previously added file assets/js/googlesitekit-modules-ads.js. You can follow an example here
      'googlesitekit-modules-search-console':
      './assets/js/googlesitekit-modules-search-console.js',

Test Coverage

  • Since datastore is not implemented yet, no tests are applicable to be added. Update any failing test, if any - like the one in assets/js/modules/index.test.js

QA Brief

  • Setup site kit without any feature flag
  • Go to Settings -> Connect more services
  • Verify that Ads module is not on the list
  • Enable adsModule feature flag
  • Go back to the Connect more services
  • Verify Ads module is present with proper icon and description
  • NOTE: There is no other functionality integrated with this module yet

Changelog entry

  • Scaffold foundational client-side infrastructure for the Ads module.
@10upsimon 10upsimon added javascript Pull requests that update Javascript code Module: Ads Google Ads module related issues P0 High priority Type: Feature New feature labels Feb 5, 2024
@bethanylang bethanylang added the Next Up Issues to prioritize for definition label Feb 5, 2024
@eugene-manuilov eugene-manuilov self-assigned this Feb 5, 2024
@eugene-manuilov
Copy link
Collaborator

@10upsimon one thing that is missing in AC is that we need to create the assets/js/googlesitekit-modules-ads.js file similarly to other module files and include it into the webpack config.

@10upsimon
Copy link
Collaborator Author

@eugene-manuilov I've updated AC to include mention of a new assets/js/googlesitekit-modules-ads.js and inclusion into the ./webpack.config.js file via the existing ./webpack/modules.config.js module config file.

@eugene-manuilov
Copy link
Collaborator

Thanks, @10upsimon. This almost looks good to me. The only thing that I forgot about in my original comment is that we need to update the Ads module class to set up assets the same way how we do it in other modules. For example:

protected function setup_assets() {
$base_url = $this->context->url( 'dist/assets/' );
$dependencies = array(
'googlesitekit-api',
'googlesitekit-data',
'googlesitekit-datastore-site',
'googlesitekit-modules',
'googlesitekit-vendor',
'googlesitekit-components',
);
$analytics_exists = apply_filters( 'googlesitekit_module_exists', false, 'analytics' );
// Note that the Tag Manager bundle will make use of the Analytics bundle if it's available,
// but can also function without it, hence the conditional include of the Analytics bundle here.
if ( $analytics_exists ) {
$dependencies[] = 'googlesitekit-modules-analytics';
}
return array(
new Script(
'googlesitekit-modules-tagmanager',
array(
'src' => $base_url . 'js/googlesitekit-modules-tagmanager.js',
'dependencies' => $dependencies,
)
),
);
}

It also means that we need to update the Ads module class to implement the Module_With_Assets interface and to use the

final class Tag_Manager extends Module
implements Module_With_Scopes, Module_With_Settings, Module_With_Assets, Module_With_Debug_Fields, Module_With_Owner, Module_With_Service_Entity, Module_With_Deactivation, Module_With_Tag {

use Module_With_Assets_Trait;

@10upsimon
Copy link
Collaborator Author

@eugene-manuilov back to you for ACR, thank you

@eugene-manuilov
Copy link
Collaborator

Thanks, @10upsimon. AC ✔️

@tofumatt
Copy link
Collaborator

IB ✅

@tofumatt tofumatt removed their assignment Feb 14, 2024
@zutigrm zutigrm self-assigned this Feb 14, 2024
@bethanylang bethanylang removed the Next Up Issues to prioritize for definition label Feb 15, 2024
@zutigrm zutigrm mentioned this issue Feb 19, 2024
18 tasks
@zutigrm zutigrm removed their assignment Feb 21, 2024
@tofumatt tofumatt assigned tofumatt and zutigrm and unassigned tofumatt Feb 26, 2024
@zutigrm zutigrm assigned tofumatt and unassigned zutigrm Feb 29, 2024
@tofumatt tofumatt removed their assignment Feb 29, 2024
@wpdarren wpdarren self-assigned this Feb 29, 2024
@wpdarren
Copy link
Collaborator

wpdarren commented Feb 29, 2024

QA Update: ❌

@zutigrm When I enable the adsModule feature flag, the Ads module appears in settings but without a logo.

image

image

I tested this on a fresh new site.

Also, since the description is a single sentence, then, we shouldn't have a period in the end like the other modules, i.e. Tag Manager. I realize it has one on the Figma designs though,

@wpdarren wpdarren assigned zutigrm and unassigned wpdarren Feb 29, 2024
@zutigrm zutigrm mentioned this issue Mar 1, 2024
18 tasks
@zutigrm
Copy link
Collaborator

zutigrm commented Mar 1, 2024

@wpdarren Thank you for your observations. It seems the assets trait got lost in one of the previous conflict syncs with develop. I sent the follow up. Also removed the dot from description

@wpdarren
Copy link
Collaborator

wpdarren commented Mar 3, 2024

QA Update: ✅

Verified:

  • The Ads module is not on the list when the adsModule feature flag is not enabled.
  • The Ads module has the icon and description when enabled by the adsModule feature flag.
  • When I tried to set up the Ads module, and went to the Connected tab, the icon appears as expected.
Screenshots

image
image
image

@wpdarren wpdarren removed their assignment Mar 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javascript Pull requests that update Javascript code Module: Ads Google Ads module related issues P0 High priority Type: Feature New feature
Projects
None yet
Development

No branches or pull requests

7 participants