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

revert: bring back gtm #423

Merged
merged 7 commits into from
Jul 25, 2021
Merged

revert: bring back gtm #423

merged 7 commits into from
Jul 25, 2021

Conversation

mazipan
Copy link
Member

@mazipan mazipan commented Jul 25, 2021

Closes #419

Description

Based on this PR #320, I revert the changes about GTM, move it to the top level

Current Tasks

@netlify
Copy link

netlify bot commented Jul 25, 2021

✔️ Deploy Preview for wargabantuwarga ready!

🔨 Explore the source changes: fd7d215

🔍 Inspect the deploy log: https://app.netlify.com/sites/wargabantuwarga/deploys/60fdbaea1441480007880128

😎 Browse the preview: https://deploy-preview-423--wargabantuwarga.netlify.app

@codecov
Copy link

codecov bot commented Jul 25, 2021

Codecov Report

Merging #423 (fd7d215) into main (55df148) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #423   +/-   ##
=======================================
  Coverage   29.56%   29.56%           
=======================================
  Files          67       67           
  Lines         866      866           
  Branches      258      258           
=======================================
  Hits          256      256           
  Misses        605      605           
  Partials        5        5           
Impacted Files Coverage Δ
pages/_app.tsx 0.00% <ø> (ø)
pages/home-page.tsx 0.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55df148...fd7d215. Read the comment docs.

@mazipan mazipan enabled auto-merge July 25, 2021 16:24
@mazipan mazipan requested a review from zainfathoni July 25, 2021 16:24
@mazipan mazipan added the need-perf-check Trigger Lighthouse-CI check label Jul 25, 2021
@baraeb92
Copy link
Collaborator

Is this something that we can add unit test to?

Just to make sure similar issue less likely happening in the future? @mazipan

@mazipan
Copy link
Member Author

mazipan commented Jul 25, 2021

I think we can cover this case by our automated testing.
Will create on seperate PR.

Since this is a high priority bug, we need to deliver the fix as fast as we can.

@mazipan
Copy link
Member Author

mazipan commented Jul 25, 2021

Create #425 for the testing part

@mazipan mazipan merged commit bdb65c3 into kawalcovid19:main Jul 25, 2021
Copy link
Collaborator

@baraeb92 baraeb92 left a comment

Choose a reason for hiding this comment

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

Just some concern about this mas @mazipan @zainfathoni

Need to make sure that the noscript is also present on the body, currently, when loading the web, it seems that there's no noscript present when I check the source code.

@@ -82,6 +83,25 @@ export default function App({ Component, pageProps, router }: AppProps) {
<link href="/manifest.json" rel="manifest" />
<meta content="#1667C2" name="theme-color" />
</Head>

Copy link
Collaborator

Choose a reason for hiding this comment

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

This line 86 could be removed I think

style={{ display: "none", visibility: "hidden" }}
title="gtm"
width="0"
/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

need to add </iframe> instead for the noscript to work properly.

See previous implementation here: https://github.com/kawalcovid19/wargabantuwarga.com/pull/36/files?file-filters%5B%5D=.tsx

Copy link
Member

Choose a reason for hiding this comment

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

in React, they should be identical, Mas @baraeb92.

But somehow it's not showing in the page now, could you please investigate it further, Mas @mazipan?

Copy link
Member Author

Choose a reason for hiding this comment

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

Bisa insert expectation kode komplit nya dari GTM gak ya, Mas @baraeb92.?

Copy link
Member Author

@mazipan mazipan Jul 26, 2021

Choose a reason for hiding this comment

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

Since all these codes are just copy-paste from previous implementation without any modifications

Copy link
Member Author

Choose a reason for hiding this comment

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

Screen Shot 2021-07-26 at 10 50 41

It already exist, it just under main tag, should we move it under body?

@mazipan mazipan deleted the mazipan/revert-gtm branch August 2, 2021 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need-perf-check Trigger Lighthouse-CI check
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Google Tag Manager is not triggered in the new homepage
5 participants