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

Update docs and code to remove GA3 references and match GA4 reality #14910

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Update docs and code to remove GA3 references and match GA4 reality #14910

wants to merge 1 commit into from

Conversation

stephaniehobson
Copy link
Contributor

@stephaniehobson stephaniehobson commented Jul 30, 2024 •
edited
Loading

One-line summary

Update analytics documentation to only reference GA4 and edit the code to make it match the GA4 docs.

Significant changes and points to review

  • Re-write analytics documentation intro
  • Remove: UA documentation
  • Remove: UA dataLayer pushes
  • Remove: data-link-group (migrate into data-link-position where appropriate)
  • Remove: data-cta-type="button"
  • Remove: data-cta-type="link"
  • data-cta-* and data-link-* should not be used on the same attribute

Issue / Bugzilla link

#13951
#14053
#14054

Testing

I know there's a lot of changes but most of them are simple edits. Hopefully this isn't too onerous!

stephaniehobson
stephaniehobson commented Jul 30, 2024
View reviewed changes
bedrock/base/templates/includes/protocol/navigation/nav-cta.html
@@ -19,7 +19,7 @@
class_name='mzp-t-product mzp-t-secondary mzp-t-md',
optional_attributes= {
'data-cta-text' : 'Get Mozilla VPN',
'data-cta-type' : 'button',
'data-cta-type' : 'vpn',
Copy link
Contributor Author

@stephaniehobson stephaniehobson Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This links to a page on WMO and so should not have the fxa- prefix other cta-types have (true for all occurrences of vpn_product_referral_link)

@stephaniehobson stephaniehobson marked this pull request as ready for review August 2, 2024 18:35
@stephaniehobson stephaniehobson added Needs Review Awaiting code review Review: S Code review time: 30 mins to 1 hour labels Aug 2, 2024
@stephaniehobson stephaniehobson marked this pull request as draft August 2, 2024 19:36
@stephaniehobson stephaniehobson removed the Needs Review Awaiting code review label Aug 2, 2024
@stephaniehobson
Copy link
Contributor Author

Note to self: add docs about stub attribution needing data-download-version

@stephaniehobson stephaniehobson mentioned this pull request Aug 20, 2024
Merged
9 tasks
@stephaniehobson stephaniehobson changed the title Update analytics documentation, and reflect docs in code Update docs and code to remove GA3 references and match GA4 reality Aug 22, 2024
@stephaniehobson stephaniehobson added the Frontend HTML, CSS, JS... client side stuff label Aug 30, 2024
stephaniehobson
stephaniehobson commented Sep 4, 2024
View reviewed changes
docs/attribution/0002-firefox-desktop.rst
@@ -8,6 +8,8 @@
Firefox desktop attribution
===========================

TODO: add something about `data-download-version` being checked to apened stub attribution.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know this needs to be done, I'd like to get the other changes merged before it gets too stale if it's okay to merge a todo.

Copy link
Contributor

Choose a reason for hiding this comment

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

So should we leave this for another PR?

@stephaniehobson stephaniehobson marked this pull request as ready for review September 4, 2024 19:14
@stephaniehobson stephaniehobson added the Needs Review Awaiting code review label Sep 4, 2024
@reemhamz
Copy link
Contributor

There seem to be quite some conflicts, so a rebase is in order! Once that's done I can get to reviewing this :)

@codecov Codecov
Copy link

codecov bot commented Sep 11, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.88%. Comparing base (f1e7b77) to head (6b002f5).

Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #14910   +/-   ##
=======================================
  Coverage   77.88%   77.88%           
=======================================
  Files         163      163           
  Lines        8480     8480           
=======================================
  Hits         6605     6605           
  Misses       1875     1875

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@stephaniehobson
Copy link
Contributor Author

Rebased!

@stephaniehobson
Update analytics documentation, and reflect docs in code
6b002f5 
Update analytics documentation to remove UA and edit the code to make it match the GA4 docs.

-    Re-write analytics documentation intro
-    Remove: UA documentation
-    Remove: UA dataLayer pushes
-    Remove: data-link-group (migrate into data-link-position where appropriate)
-    Remove: data-cta-type="button"
-    Remove: data-cta-type="link"
-    data-cta and data-link should not be used on the same attribute
@stephaniehobson
Copy link
Contributor Author

Rebased again!

@reemhamz reemhamz self-assigned this Sep 20, 2024
reemhamz
reemhamz requested changes Sep 24, 2024
View reviewed changes
Copy link
Contributor

@reemhamz reemhamz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Whewwwww, this was a big one.

💡 Note that this needs another rebase

Good job working so diligently on this! I left some important questions and suggestions, however I'm also still finding instances of data-cta-type="b utton", data-cta-type="link", and data-link-group in the code in the code that should be addressed:

Screenshot 2024-09-24 at 1 29 58 PM

Screenshot 2024-09-24 at 1 29 34 PM

Screenshot 2024-09-24 at 1 28 40 PM

bedrock/base/templates/includes/protocol/navigation/menus-refresh/about-us.html
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: You mentioned in the PR info that we're removing data-link-group and (if appropriate) replacing it with data-link-position, but I see both of those attributes on this file?

bedrock/base/templates/includes/protocol/navigation/menus-refresh/firefox.html
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Same as above, I see both data-link-group with data-link-position in this file

bedrock/base/templates/includes/protocol/navigation/menus-refresh/products.html
Copy link
Contributor

Choose a reason for hiding this comment

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

data-link-group with data-link-position in this file

bedrock/firefox/templates/firefox/features/includes/subnav.html
Copy link
Contributor

Choose a reason for hiding this comment

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

This file isn't being used anywhere?

docs/attribution/0001-analytics.rst
does not allow for the injection of custom HTML or JavaScript but all tags use
built in templates to minimize any chance of introducing a bug into Bedrock.
Google calls a 3rd party JavaScript library that is imported by adding a script tag to a website,
Google names these "tag"s.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Google names these "tag"s.
Google names these "tags".

docs/attribution/0001-analytics.rst
action: 'clickthrough',
name: '<banner name>', //ex. Fb-Video-Compat
});
Some form submissions are also being collected but newsletter signups are not. `(See Bug #13348)`_ They are instead
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Some form submissions are also being collected but newsletter signups are not. `(See Bug #13348)`_ They are instead
Some form submissions are also being collected but newsletter signups are not `(see bug #13348)`_ . They are instead

docs/attribution/0001-analytics.rst
variant: 'campaign-page',
});
}
The attribute ``data-cta-text`` must be present to trigger the event. All links to accounts.firefox.com must also use cta-type.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The attribute ``data-cta-text`` must be present to trigger the event. All links to accounts.firefox.com must also use cta-type.
The attribute ``data-cta-text`` must be present to trigger the event. All links to accounts.mozilla.com must also use ``datacta-type`` .

docs/attribution/0001-analytics.rst
+-----------------------+------------------------------------------------------------------------------------------------+
| Data Attribute | Expected Value |
+=======================+================================================================================================+
| ``data-link-text`` * | Text or name of the link (e.g. ``Monitor``, ``Features``, ``Instagram (mozilla)``, |
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: What is this asterisks referring to?

docs/attribution/0001-analytics.rst
+-----------------------+----------------------------------------------------------------------------------+
| Data Attribute | Expected Value |
+=======================+==================================================================================+
| ``data-cta-text`` * | Text or name of the link (e.g. ``Sign Up``, ``Start Here``, ``Get Relay``, |
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: What is this asterisks referring to?

docs/attribution/0002-firefox-desktop.rst
@@ -8,6 +8,8 @@
Firefox desktop attribution
===========================

TODO: add something about `data-download-version` being checked to apened stub attribution.
Copy link
Contributor

Choose a reason for hiding this comment

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

So should we leave this for another PR?

@reemhamz reemhamz removed the Needs Review Awaiting code review label Sep 24, 2024
@reemhamz reemhamz mentioned this pull request Oct 1, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reemhamz reemhamz reemhamz requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@reemhamz reemhamz

Labels
Analytics 📈 Frontend HTML, CSS, JS... client side stuff Review: S Code review time: 30 mins to 1 hour
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@stephaniehobson @reemhamz