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.

Sign up for GitHub

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

Jump to bottom

feat: add Google Analytics Event trigger #3125

Merged
merged 4 commits into from
Jun 19, 2023
Merged

feat: add Google Analytics Event trigger #3125

merged 4 commits into from
Jun 19, 2023

Conversation

neatbyte-vnobis
Copy link
Collaborator

Changes

Implements issue "Add a trigger for Google Analytics / Google Ad events".
WIP

How Has This Been Tested?

Manual

Documentation

None

@neatbyte-vnobis
Copy link
Collaborator Author

neatbyte-vnobis commented Mar 21, 2023 •
edited
Loading

@SvenAlHamad @Pavel910 @adrians5j
I want to confirm/ask few things before you will review this PR.

  1. Should users add googletagmanager script somewhere inside apps/website/public/index.html?
    (I mean manually add a <script /> tag there with proper Gtag script path with G-XXX... id)
    Or there is other way to configure Google Analytics via UI?

  2. Can I use @types/gtag.js package for gtag typescript type?
    Currently I declared gtag here via stab to bypass typescript type warnings.

  3. Also wanted to clarify one thing about UI component:
    In requirements there is a reference to SEOSettingsView component.
    packages/app-page-builder/src/editor/ui/views/SEOSettingsView.ts
    But this is class component with no exposed callback/control interface, so it cannot be reused.
    And as I know we are not going to use class components in future.
    I have found a component which has similar functionality - it is used for webhook trigger WebhooksRequestsDynamicFieldset.
    packages/app-form-builder/src/admin/plugins/editor/triggers/components/WebhooksRequestsDynamicFieldset.tsx
    It only lacks ability to add new rows after specific row (it always adds to the end of array).
    So I decided to use this component as a reference and just add one more field to the rows.
    Just let me know if it is ok. Screenshot bellow:
    image

@adrians5j adrians5j self-assigned this Mar 22, 2023
@SvenAlHamad
Copy link
Contributor

@neatbyte-vnobis Regarding question #1 - Let's not insert GA into the page automatically. Let's just add a note under that trigger saying: "Make sure you have Google Analytics installed on your website before using this trigger.".
I'll let Pavel and Adrian comment on other questions.

@SvenAlHamad SvenAlHamad reopened this Mar 22, 2023
@adrians5j
Copy link
Member

I'll comment on this tomorrow morning (I've already spent my daily PR-reviews time box 😉).

@adrians5j
Copy link
Member

Question #2 - sounds good 👍🏻

Question #3 - also sounds good 👍🏻

Thank you @neatbyte-vnobis !

@neatbyte-vnobis
Copy link
Collaborator Author

@SvenAlHamad
Please take a look on 2 images. Which one is looking better for you?

1.

image

2.

image

@SvenAlHamad
Copy link
Contributor

@neatbyte-vnobis Let's use Option #1, but I would place the message under the button.

@neatbyte-vnobis
Copy link
Collaborator Author

@SvenAlHamad PR was updated according to a comment

@neatbyte-vnobis
Copy link
Collaborator Author

@SvenAlHamad @adrians5j
Any updates on this PR?
Maybe it worth to merge latest next into it and retest it

adrians5j
adrians5j requested changes Jun 13, 2023
View reviewed changes
Copy link
Member

@adrians5j adrians5j left a comment

Choose a reason for hiding this comment

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

Looks good, just need to double-check frontend trigger processing.

apps/theme/layouts/forms/DefaultFormLayout.tsx Outdated
@@ -52,6 +52,17 @@ const DefaultFormLayout: FormLayoutComponent = ({
const result = await submit(data);
setLoading(false);
if (result.error === null) {
const googleAnalyticsEvent = formData.triggers?.["google-analytics-event"];
Copy link
Member

Choose a reason for hiding this comment

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

Trigger processing should not be in the layout. Triggers are handled internally, here:

packages/app-form-builder/src/components/Form/FormRender.tsx:173

@adrians5j adrians5j merged commit e42eda1 into next Jun 19, 2023
@adrians5j adrians5j added this to the 5.37.0 milestone Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adrians5j adrians5j adrians5j approved these changes

Assignees

@adrians5j adrians5j

Labels
project-upgrade-ready
Projects
None yet
Milestone
5.37.0
Development

Successfully merging this pull request may close these issues.
3 participants
@neatbyte-vnobis @SvenAlHamad @adrians5j