feat: Preserve search parameters in external links #258

DiogoSoaress · 2023-12-12T10:56:33Z

What it solves

We want to preserve the search parameters when navigating the homepage.

How this PR fixes it

On page navigation the search parameters are merged and stored in the session storage key SAFE__searchParams and saved to a Context.
All links navigating to https://app.safe.global include the stored search parameters.

github-actions · 2023-12-12T10:57:34Z

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type	Occurrences	Fixable
Errors	0	0
Warnings	0	0
Ignored	0	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

github-actions · 2023-12-12T10:58:19Z

Branch preview

✅ Deployed to dev:

https://safe-web-landing.dev.5afe.dev

DiogoSoaress · 2023-12-12T10:58:53Z

I would extend this behaviour to the other links in the homepage if you could navigate to a campaign page from https://safe.global , otherwise I don't see a justification to attach campaign related UTM codes when navigating to https://app.safe.global/

katspaugh · 2023-12-12T11:52:21Z

I would do it for all pages, not just campaigns
Store the initial UTM params in the session storage so that they aren't gone on page refresh
Why create a new component? You could just add it directly to ExternalLink.
Next.js has a hook to get parsed params, which AFAIK is now preferred to useRouter, check out https://nextjs.org/docs/app/api-reference/functions/use-search-params

DiogoSoaress · 2023-12-13T10:23:38Z

I would do it for all pages, not just campaigns

Why?

Store the initial UTM params in the session storage so that they aren't gone on page refresh

The search params are not lost on page refresh. This approach is however necessary if we want to hydrate the links on all pages with the UTM params, right?

katspaugh · 2023-12-13T10:41:18Z

Because the user can:

come to a campaign page from a twitter link
navigate to another page on the site
2a. (potentially refresh the page)
only then go to app.safe.global

We lose the UTM params on steps 2 and 2a in your current implementation. Refreshing the page is less likely but navigating within the website is very likely.

DiogoSoaress · 2023-12-13T11:07:23Z

To clarify on the campaigns navigation:

the user cannot reach the campaigns from safe.global and only the buttons in the campaign page link to the custom /welcome route in app.safe.global. The links on other pages link to https://app.safe.global, see https://safe.global/wallet for example.

if you are in the current implementation with the UTM params shared on Twitter https://safe-web-landing.dev.5afe.dev/campaigns/social-login-gnosis?utm_source=t.co&utm_medium=cpc&utm_campaign=seedlessv1 and refresh the page, the search params are preserved.

Am I missing / overlooking something?

katspaugh · 2023-12-13T11:25:41Z

Once again: the user can navigate to another internal page from the campaign page (e.g. via the links in the footer), at which point UTM params are lost.
I just don't see the point of limiting this solution to one particular type of pages while also making it less robust.

katspaugh · 2023-12-13T11:33:28Z

I'll outline again my proposed solution which I shared before you started working on this PR:

On page load, in a global context, save UTM params into the context and sessionStorage.
Also attempt to restore any existing params from sessionStorage in case they are not in the URL.
For all app.safe.global links, read UTMs from the global context and append them to the link.

If you still disagree, please clarify the requiements with BI and marketing.

katspaugh

Importing the context, the append function and calling said function for every link in every component with external links isn’t very maintainable don’t you think? Please propose an alternative solution to discuss.

The context needs tests.

DiogoSoaress · 2023-12-14T08:48:11Z

Please propose an alternative solution to discuss.

I think having a HOC to call the context and append the search params is more maintainable, avoiding all the duplicated code.
You mentioned above to use the ExternalLink component but we don't have that component in this repo so I need to create a new component for this purpose. WDYT?

katspaugh · 2023-12-14T08:55:26Z

I don't see the need for a HoC. Just create a custom link component, e.g. SafeLink that has all the logic and renders a Link.

katspaugh · 2023-12-14T14:50:23Z

src/components/common/SafeLink/index.tsx

+  const finalHref = appendSearchParamsToURL(href, searchParams)
+
+  return (
+    <Link href={finalHref} target="_blank" rel="noreferrer">


We should remove rel="noreferrer" for app.safe.global links.

katspaugh · 2023-12-14T14:50:58Z

src/components/common/SafeLink/index.tsx

+const SafeLink = ({ href, children }: { href: string; children: ReactNode }) => {
+  const searchParams = useContext(SearchParamsContext)
+
+  const finalHref = appendSearchParamsToURL(href, searchParams)


useMemo perhaps?

katspaugh · 2023-12-14T14:52:24Z

Looks good to me.

@usame-algan could you also take a look when you have a minute?

usame-algan

Looks good 👍

I assume we use SafeLink for all links to app.safe.global but haven't checked each occurence.

Although for the Launch Wallet button in the header it doesn't seem to be used. Is that intended?

src/components/Campaign/Hero/styles.module.css

DiogoSoaress · 2023-12-14T15:35:10Z

Launch Wallet button in the header it doesn't seem to be used

I've included it inside of the ContextProvider. Good catch @usame-algan

katspaugh

Thanks Diogo.
In future reviews we should keep in mind that we should always use SafeLink instead of Link for all links except when 100% internal.

DiogoSoaress · 2023-12-14T15:50:28Z

Definitely!
I am very curious to see how carrying the UTM parameters will give another dimension to our website and wallet app traffic.

DiogoSoaress added 5 commits December 11, 2023 13:19

fix: CSS warning

9c30c3c

feat: LinkHOC

a53be79

do not use next Link

bed21d1

feat: extract mergeSearchParams to util fn

7f99e6a

feat: switch link component in HeaderCTA

1d912f6

fix test

27ed162

DiogoSoaress requested a review from katspaugh December 12, 2023 11:06

DiogoSoaress added 6 commits December 13, 2023 13:04

prefer useSearchParams over useRouter

a6d791c

group Contexts in dedicated folder

4b09477

feat: SearchParamsContext and store the search params in session storage

a126b99

append search params to remaining app.safe.global links

8cf19a2

remove ExternalLinkHOC

54cd336

revert unnecessary changes

62db557

DiogoSoaress force-pushed the pass-utm-params branch from dff062d to 62db557 Compare December 13, 2023 23:22

DiogoSoaress changed the title ~~feat: Pass UTM parameters to campaign links~~ feat: Pass UTM parameters to external links in the homepage Dec 13, 2023

DiogoSoaress changed the title ~~feat: Pass UTM parameters to external links in the homepage~~ feat: Preserve search parameters in external links Dec 13, 2023

katspaugh requested changes Dec 14, 2023

View reviewed changes

DiogoSoaress added 2 commits December 14, 2023 12:41

feat: component SafeLink

cdeca16

tests: SearchParamsContext

3f835be

DiogoSoaress requested a review from katspaugh December 14, 2023 14:47

katspaugh reviewed Dec 14, 2023

View reviewed changes

katspaugh requested a review from usame-algan December 14, 2023 14:51

memoize href on SafeLink

fe82e49

usame-algan approved these changes Dec 14, 2023

View reviewed changes

src/components/Campaign/Hero/styles.module.css Show resolved Hide resolved

include Header in the SearchParamsContext

c64400d

katspaugh approved these changes Dec 14, 2023

View reviewed changes

DiogoSoaress merged commit 173c225 into main Dec 14, 2023
5 checks passed

DiogoSoaress deleted the pass-utm-params branch December 14, 2023 15:50

github-actions bot locked and limited conversation to collaborators Dec 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Preserve search parameters in external links #258

feat: Preserve search parameters in external links #258

DiogoSoaress commented Dec 12, 2023 •

edited

Loading

github-actions bot commented Dec 12, 2023 •

edited

Loading

github-actions bot commented Dec 12, 2023 •

edited

Loading

DiogoSoaress commented Dec 12, 2023

katspaugh commented Dec 12, 2023

DiogoSoaress commented Dec 13, 2023 •

edited

Loading

katspaugh commented Dec 13, 2023 •

edited

Loading

DiogoSoaress commented Dec 13, 2023

katspaugh commented Dec 13, 2023

katspaugh commented Dec 13, 2023 •

edited

Loading

katspaugh left a comment

DiogoSoaress commented Dec 14, 2023

katspaugh commented Dec 14, 2023

katspaugh Dec 14, 2023

katspaugh Dec 14, 2023

katspaugh commented Dec 14, 2023

usame-algan left a comment •

edited

Loading

DiogoSoaress commented Dec 14, 2023

katspaugh left a comment

DiogoSoaress commented Dec 14, 2023

feat: Preserve search parameters in external links #258

feat: Preserve search parameters in external links #258

Conversation

DiogoSoaress commented Dec 12, 2023 • edited Loading

What it solves

How this PR fixes it

github-actions bot commented Dec 12, 2023 • edited Loading

ESLint Summary View Full Report

github-actions bot commented Dec 12, 2023 • edited Loading

Branch preview

DiogoSoaress commented Dec 12, 2023

katspaugh commented Dec 12, 2023

DiogoSoaress commented Dec 13, 2023 • edited Loading

katspaugh commented Dec 13, 2023 • edited Loading

DiogoSoaress commented Dec 13, 2023

katspaugh commented Dec 13, 2023

katspaugh commented Dec 13, 2023 • edited Loading

katspaugh left a comment

Choose a reason for hiding this comment

DiogoSoaress commented Dec 14, 2023

katspaugh commented Dec 14, 2023

katspaugh Dec 14, 2023

Choose a reason for hiding this comment

katspaugh Dec 14, 2023

Choose a reason for hiding this comment

katspaugh commented Dec 14, 2023

usame-algan left a comment • edited Loading

Choose a reason for hiding this comment

DiogoSoaress commented Dec 14, 2023

katspaugh left a comment

Choose a reason for hiding this comment

DiogoSoaress commented Dec 14, 2023

DiogoSoaress commented Dec 12, 2023 •

edited

Loading

github-actions bot commented Dec 12, 2023 •

edited

Loading

github-actions bot commented Dec 12, 2023 •

edited

Loading

DiogoSoaress commented Dec 13, 2023 •

edited

Loading

katspaugh commented Dec 13, 2023 •

edited

Loading

katspaugh commented Dec 13, 2023 •

edited

Loading

usame-algan left a comment •

edited

Loading