[WNMGDS-3344] Add Utag data object #3559
Conversation
jack-ryan-nava-pbc
added
the
Impacts: Documentation
|
@kim-cmsds @tamara-corbalt This change will necessitate some changes to the Gatsby-Head component in the React 18 branch, but lets hold off doing those changes until Blast confirms these changes. |
packages/docs/gatsby-ssr.tsx
Outdated
| a=b.getElementsByTagName(c)[0];a.parentNode.insertBefore(d,a); | ||
| })();`; | ||
|
|
||
| const dangerousTealiumSnippet = makeDangerousHTML(tealiumSnippet); |
There was a problem hiding this comment.
Non-blocking, but I’m wondering if we need the dedicated makeDangerousHTML function here. Since it’s only used once, would it be worth inlining it?
dangerousTealiumSnippet = { __html: tealiumSnippet }
There was a problem hiding this comment.
yeah that's a good call. this is more an homage to all the work I did in a previous effort, only to delete. but I shouldn't be so sentimental. thank you!
There was a problem hiding this comment.
@jack-ryan-nava-pbc It looks good, but I'm having trouble verifying the testing steps. When I step through with the debugger, I'm seeing the link event object, but not seeing the fields view or page_name.
|
@tamara-corbalt did you get the video of the testing steps I shared via Google Docs? |
Yes, and I was able to walk through the testing steps. Thanks for sharing the screencast! Everything looks like it's working as expected, so hitting approve! |
| <script type="text/javascript" dangerouslySetInnerHTML={content}></script> | ||
| ); | ||
|
|
||
| exports.onRenderBody = ({ setPreBodyComponents }) => { |
There was a problem hiding this comment.
TIL (/ re-remembered) about this gatsby feature
| @@ -66,12 +69,45 @@ const Layout = ({ | |||
| theme, | |||
| tableOfContentsData, | |||
| }: LayoutProps) => { | |||
| const env = 'prod'; | |||
| // Using a ref here because this value shouldn't change after the initial render. | |||
There was a problem hiding this comment.
thanks for the comment here
| page_name: tabTitle, | ||
| page_type: tabTitle.includes('Page not found') ? 'true' : 'false', //If page is a 404 (error page) this is set to true, otherwise it is false | ||
| site_environment: env.current, //Used to include or exclude traffic from different testing environments. Ex: test, test0, imp, production | ||
| site_section: location.pathname == '/' ? 'index' : location.pathname, // Set the section to the pathname, except in the case of the index. |
There was a problem hiding this comment.
NB: I don't think this matters but why use two equals signs and not three for the "index" check?
There was a problem hiding this comment.
So this is me trying out a sorta new thing for me: I've adopted the opinion that if you use == it means you know what the types are in the comparison you are making. The only difference between === and == is that === checks to see if the two items being compared are the same type at first.
So using == here states that we know that location.pathname will only ever return a string so we don't need the type comparison to another string value.
There was a problem hiding this comment.
Is using == to signal confidence in types a widely adopted convention? I haven’t come across that before, so I’m wondering if there’s a more explicit or widely recognized way to communicate type confidence to folks reading the code.
There was a problem hiding this comment.
It isn't, but I think that's ok. Also, I boogered up my explanation a little bit. Here's a good reference and a good quote from that reference: "The implication here then is that both == and === check the types of their operands. The difference is in how they respond if the types don't match."
So basically what I'm saying with this code is coercion is ok here because I know the types are the same, so no coercion occurs. Maybe this isn't the most thought-out application of the principle I intended to apply, but it should function fine. And clearly I should think more (and review) about this pattern going forward. Thanks for pushing me to think more @tamara-corbalt!
* Add ssr helper * Make HTML dangerous * Seems like it works * Use ref for variable storage and remove unneeded function
Summary
How to test
Checklist
[WNMGDS-####] Titleor [NO-TICKET] if this is unticketed work.Type(only one) label for this PR, if it is a breaking change, label should only beType: BreakingImpacts, multiple can be selected.