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 global GTM container #2128

Merged
merged 1 commit into from
Oct 9, 2020
Merged

Add global GTM container #2128

merged 1 commit into from
Oct 9, 2020

Conversation

randyzwitch
Copy link
Contributor

As part of the analytics implementation work, adding a second GTM container for development purposes. This will allow implementors to create rules separate from current tracking. When cut-over ready to happen, can submit another PR to remove the original GTM container

@@ -25,6 +25,17 @@
})(window, document, 'script', 'dataLayer', 'GTM-TXFLBCM');</script>
<!-- End Google Tag Manager -->

<!-- Google Tag Manager -->
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a comment in-line here, to explain why we're doing two GTMs?

(It seems a little weird to duplicate the exact code instead of just passing in the GTM id ('GTM-52GRQSL') into a reusable function, but on reflection this is really short anyways and is an official copy+paste So I think we should keep as-is)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is literally here for a week, possibly less. does it really need commentary?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I thought this was going to be around longer than that. NVM then!

@randyzwitch randyzwitch merged commit 4a92ee8 into streamlit:develop Oct 9, 2020
@randyzwitch randyzwitch deleted the add_global_gtm_container branch October 9, 2020 11:57
tconkling added a commit to tconkling/streamlit that referenced this pull request Oct 12, 2020
# By karrie (7) and others
# Via GitHub
* develop:
  Removing cache option from main menu if s4a (streamlit#2149)
  Fix empty deploy page (streamlit#2148)
  Don't wait for unit tests before starting Cypress (streamlit#2142)
  Fix formatting of st.file_uploader docstring (streamlit#2141)
  Fix broken link in 0.68 changelog (streamlit#2144)
  Fix useEffect warning (streamlit#2137)
  Add global GTM container (streamlit#2128)
  Allow Streamlit server to handle Range Requests (streamlit#1967)
  rename hosted to hostedAt (streamlit#2132)
  Update change log
  Update notices
  Up version to 0.68.0
  Rename hosted to hostedAt in tracking data (streamlit#2132)
  Inject tracking data (streamlit#2110)
  [Feature Branch] File uploader (streamlit#2130)
  links for docs (streamlit#2129)
  Upgrade ProtobufJS and fix build script (streamlit#2118)
  Refresh landing page (streamlit#2116)
  Improve docstrings + tutorials for Layout (streamlit#2117)
  Better 'streamlit run' error message when no extension provided (streamlit#2115)

# Conflicts:
#	frontend/src/components/elements/Video/Video.tsx
#	frontend/src/components/widgets/FileUploader/FileUploader.test.tsx
#	frontend/src/components/widgets/FileUploader/FileUploader.tsx
#	frontend/src/lib/utils.ts
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.

None yet

2 participants