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 referrer to trial and demo requests #1369

Merged
merged 11 commits into from
Sep 2, 2024
Merged

Add referrer to trial and demo requests #1369

merged 11 commits into from
Sep 2, 2024

Conversation

dtuite
Copy link
Member

@dtuite dtuite commented Aug 30, 2024

No description provided.

@dtuite dtuite self-assigned this Aug 30, 2024
@dtuite
Copy link
Member Author

dtuite commented Aug 30, 2024

@@ -15,6 +15,7 @@
"@algolia/autocomplete-theme-classic": "^1.5.1",
"@headlessui/react": "^1.4.1",
"@heroicons/react": "^1.0.4",
"@reach/router": "^1.3.4",
Copy link
Member Author

Choose a reason for hiding this comment

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

This is already a dependency of gatsby. I pulled it in so I can use useLocation in the <Link /> component.

const stringSearchParams = new URLSearchParams(searchParams).toString();

if (ctaTo.includes('?')) {
return `${ctaTo}&${stringSearchParams}`;
Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a better way to do this than conditionally appending & or ??

@@ -23,7 +23,7 @@ const RequestTrial = ({ data, location }) => {
/>

<div className="min-h-screen bg-white">
<SitewideHeader ctaText="Request a Demo" ctaTo="/request-demo/" />
<SitewideHeader ctaText="Request a Demo" />
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the default in the <SitewideHeader />

@@ -111,7 +115,7 @@
"test": "cypress run",
"lint": "eslint src",
"preview": "yarn run build && netlify deploy --dir public",
"test:unit": "mocha --require @babel/register src/**/*.test.js",
"test:unit": "jest",
Copy link
Member Author

Choose a reason for hiding this comment

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

I switched all unit tests to jest instead of mocha because I needed to use full DOM rendering (rather than shallow rendering) in the tests of the <Link /> component. Jest has better integrations with JSDOM for doing this. There wasn't many unit tests in the site so it didn't take long.

@@ -48,6 +73,8 @@ const Link = ({
forceOpenInSameTab = false,
...rest
}) => {
const location = useLocation();
Copy link
Member Author

Choose a reason for hiding this comment

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

Now that this component uses a hook which relies on looking up the location on the window object, we need to provide a window like environment for testing. Hence the switch to jest and JSDOM for the tests of this component. They are probably better tests now anyway, since they run in an environment more like userland.

@dtuite dtuite marked this pull request as ready for review August 31, 2024 10:31
@dtuite dtuite requested a review from a team August 31, 2024 10:34
Copy link

sonarcloud bot commented Aug 31, 2024

@dtuite dtuite merged commit cdfb7aa into main Sep 2, 2024
6 checks passed
@dtuite dtuite deleted the demo-referrer branch October 30, 2024 10:42
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.

2 participants