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

GH-2086, GH-2084, GH-2040: Update intro hub Get Plus, Midnight & Insights Modal CTA link #595

Merged
merged 8 commits into from Aug 14, 2020

Conversation

@benstrumeyer
Copy link
Contributor

@benstrumeyer benstrumeyer commented Aug 12, 2020

  • Have you followed the guidelines in CONTRIBUTING.md?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you added an explanation of what your changes do?
  • Does your submission pass tests?
  • Did you lint your code prior to submission?
  • Intro Hub Get Plus should go to the plus description page
  • Intro Hub's Ghostery Midnight modal should link to the landing page
  • Intro Hub's Ghostery Insights modal should link to the landing page

Tickets:

…ery Midnight Modal CTA to link to landing page
Copy link
Member

@wlycdgr wlycdgr left a comment

LGTM

Copy link
Member

@christophertino christophertino left a comment

We should use www.${DOMAIN}.com here as well to point at the staging website.

@wlycdgr This probably exists in lots of places.

@fcjr
Copy link
Member

@fcjr fcjr commented Aug 13, 2020

We should use www.${DOMAIN}.com here as well to point at the staging website.

@wlycdgr This probably exists in lots of places.

While we are at it we should update the promo component to get the domains from Globals instead setting it at the top of this file.

@fcjr fcjr mentioned this pull request Aug 13, 2020
0 of 5 tasks complete
@benstrumeyer benstrumeyer requested review from wlycdgr and christophertino Aug 13, 2020
@wlycdgr
Copy link
Member

@wlycdgr wlycdgr commented Aug 13, 2020

We should use www.${DOMAIN}.com here as well to point at the staging website.

@wlycdgr This probably exists in lots of places.

@christophertino I'll check other places. We originally implemented it that way in this component & changed it by mistake, so hopefully it will not be so many.

app/shared-components/PromoModal/PromoModal.jsx Outdated Show resolved Hide resolved
app/shared-components/PromoModal/PromoModal.jsx Outdated Show resolved Hide resolved
@benstrumeyer benstrumeyer requested a review from ghostery/ghostery as a code owner Aug 13, 2020
@fcjr fcjr changed the title GH-2086, GH-2084: Update intro hub Get Plus CTA link and Midnight Modal CTA link GH-2086, GH-2084, GH-2040: Update intro hub Get Plus, Midnight & Insights Modal CTA link Aug 13, 2020
@benstrumeyer benstrumeyer requested review from fcjr and wlycdgr Aug 13, 2020
Copy link
Member

@wlycdgr wlycdgr left a comment

lgtm

@fcjr
Copy link
Member

@fcjr fcjr commented Aug 13, 2020

looks good to me, however one thing with using GHOSTERY_BASE_URL is that both these links will always result in an immediate redirect ghostery.com -> www.ghostery.com. Not sure if it matters but wanted to point it out incase @wlycdgr or @christophertino have any concerns with that

@wlycdgr
Copy link
Member

@wlycdgr wlycdgr commented Aug 13, 2020

looks good to me, however one thing with using GHOSTERY_BASE_URL is that both these links will always result in an immediate redirect ghostery.com -> www.ghostery.com. Not sure if it matters but wanted to point it out incase @wlycdgr or @christophertino have any concerns with that

In Chrome I don't see a user-visible redirect at all; in Firefox, there is one in the address bar, but there's no flicker, so I have no issue with it from a UX perspective anyway. I did notice a bug in Firefox though where the panel does not close after the Midnight landing page opens! I'll make a ticket. I made a ticket.

@christophertino
Copy link
Member

@christophertino christophertino commented Aug 14, 2020

It shouldn't matter since the request is redirected in nginx. If we wanted to be really verbose we could make another global variant as a www url, but I don't think it's necessary.

@christophertino christophertino added this to the 8.5.3 milestone Aug 14, 2020
@christophertino christophertino merged commit 7e0a3c2 into develop Aug 14, 2020
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@christophertino christophertino deleted the GH-2086 branch Aug 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants