-
Notifications
You must be signed in to change notification settings - Fork 190
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
WD-9610 Rebrand Raspi download thank you #13712
WD-9610 Rebrand Raspi download thank you #13712
Conversation
5feaaf5
to
ad430ab
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## download-bubble-wd-8482 #13712 +/- ##
========================================================
Coverage 74.49% 74.49%
========================================================
Files 107 107
Lines 2854 2854
Branches 954 954
========================================================
Hits 2126 2126
Misses 704 704
Partials 24 24 |
ba4a323
to
e41ff38
Compare
Nice job, LGTM! |
I found a weird behavior while testing. Whevener you click on "Subscribe to our newsletter" it redirects you to /download/server page no matter if you are downloading desktop, server or core image. Even if I go to /download/desktop and download desktop image I am still redirected to /download /server. I think this is incorrect. Depending on the image you are downloading you should be redirected to the corresponding page OR to the page that is common to all of them, like landing download page. @juanruitina wdyt? |
Indeed, @akbarkz, all that should make sense. I've also noticed that we are also using the server form partial in /desktop/thank-you. Let me inquire about which specific forms they should be pointing to. |
92d1640
to
b6dbb5c
Compare
b6dbb5c
to
c6bc3e4
Compare
@akbarkz @juanruitina Yeah, that should redirect to their respective pages. I've updated the partial to have their respective "returnUrls" |
c6bc3e4
to
c73406e
Compare
Found two bugs:
|
<div> | ||
<h2>Sign up for our newsletter</h2> | ||
<p>Get the latest Ubuntu news and updates in your inbox.</p> | ||
</div> | ||
|
||
<form action="/marketo/submit" method="post" id="mktoForm_4960" onsubmit="ga('send', 'Newsletter', 'Signup', 'Ubuntu Server download newsletter signup');"> | ||
<label for="email">Email*:</label> | ||
<input required id="email" name="email" maxlength="255" type="email" pattern="^[^ ]+@[^ ]+\.[a-z]{2,26}$" /> | ||
|
||
<div class="p-section--shallow"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just file rename, and added the returnUrl
parameter, nothing's changed.
c73406e
to
61327df
Compare
|
The partial doesn't matter, it's something we devs use to refactor code, I could give it any name, what you may need to inquire is the formId, but I believe we use the same formId everywhere, if we want to change it, it may need changing for the whole site, blogs and other places. I suppose Marketing would need to create campaigns for each of them too. |
Looks good now @carkod! Thanks for fixing existing bugs too, this are the most important pages on our website, so we need to make sure it works smoothly. |
Yes my understanding is that this check is to actually allow us to send newsletters. |
61327df
to
f416def
Compare
@@ -113,7 +113,7 @@ | |||
<div class="u-fixed-width"> | |||
<div class="p-notification--positive u-no-margin--bottom"> | |||
<div class="p-notification__content"> | |||
<p class="p-notification__message">Thank you for signing up for our newsletter!<br/>In these regular emails you will find the latest updates about Ubuntu and upcoming events where you can meet our team.<a href="#" onclick="location.href = document.referrer; return false;"><i class="p-notification__close">Close</i></a></p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lizzochek when you are back, can you check what removing this could break? Thanks, let me know if you need help with a better solution. I don' t think applying this globally is good, maybe we should encpasulate it within some function so that it triggers only in that specific situation and also it'd be easier to track where this code is used.
f416def
to
93ec17f
Compare
QA
Issue / Card
Fixes https://warthogs.atlassian.net/browse/WD-9610
Screenshots (design review)
Help
QA steps - Commit guidelines