Issue 29322769: Issue 2844 - Create Microsoft Edge coming soon landing page

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+301 lines, -19 lines)			Patch
M	includes/index.tmpl	View	1 2 3 4 5 6 7 8 9 10 11	6 chunks	+35 lines, -0 lines	0 comments	Download
R	pages/adblock-browser/verification-success.html	View	1 2 3	1 chunk	+0 lines, -5 lines	0 comments	Download
A	pages/edge.md	View		1 chunk	+1 line, -0 lines	0 comments	Download
A	pages/verification-success.html	View	1 2 3	1 chunk	+5 lines, -0 lines	0 comments	Download
M	static/css/index.css	View	1 2 3 4 5 6 7 8 9	4 chunks	+88 lines, -3 lines	0 comments	Download
M	static/css/index-desktop.css	View	1 2 3 4 5 6 7 8 9	3 chunks	+30 lines, -3 lines	0 comments	Download
M	static/css/index-mobile.css	View	1 2 3 4 5 6 7 8 9	4 chunks	+19 lines, -3 lines	0 comments	Download
A	static/img/edge_teaser.png	View	1 2 3	Binary file		0 comments	Download
M	static/img/sprite-index.png	View	1 2 3 4 5 6 7 8 9 10	Binary file		0 comments	Download
M	static/js/index.js	View	1 2 3 4 5 6 7 8 9 10 11	3 chunks	+123 lines, -5 lines	0 comments	Download

Messages

Total messages: 20

Expand All Messages | Collapse All Messages

saroyanm

Please also process Coming soon image with pngout https://codereview.adblockplus.org/29322769/diff/29323520/includes/index.tmpl File includes/index.tmpl (right): https://codereview.adblockplus.org/29322769/diff/29323520/includes/index.tmpl#newcode36 includes/index.tmpl:36: <script ...

Aug. 20, 2015, 8:54 p.m. (2015-08-20 20:54:54 UTC) #3

Oleksandr

https://codereview.adblockplus.org/29322769/diff/29323520/includes/index.tmpl File includes/index.tmpl (right): https://codereview.adblockplus.org/29322769/diff/29323520/includes/index.tmpl#newcode118 includes/index.tmpl:118: params += "&edge=" + "true"; On 2015/08/20 20:54:53, saroyanm ...

Aug. 26, 2015, 12:19 a.m. (2015-08-26 00:19:30 UTC) #4

https://codereview.adblockplus.org/29322769/diff/29323520/includes/index.tmpl
File includes/index.tmpl (right):

https://codereview.adblockplus.org/29322769/diff/29323520/includes/index.tmpl...
includes/index.tmpl:118: params += "&edge=" + "true";
On 2015/08/20 20:54:53, saroyanm wrote:
> We were not passing parameter for Adblock Browser subscribers. Should we also
> update Adblock Browser Beta form ?

No. Adblock Browser is default.

https://codereview.adblockplus.org/29322769/diff/29323520/includes/index.tmpl...
includes/index.tmpl:183: {{"Get a heads-up prior to the
launch!"|translate("ios-subscription-description")}}
On 2015/08/20 20:54:53, saroyanm wrote:
> I assume the text was copy pasted, please change the message ID it shouldn't
be
> "ios-subscription-description"
> Same goes for other messageIDs below.

Done. I assumed we could reuse the ios translations, but it doesn't look like
that's possible.

https://codereview.adblockplus.org/29322769/diff/29323520/includes/index.tmpl...
includes/index.tmpl:487: {{"Microsoft Edge <a
href='https://wpdev.uservoice.com/forums/257854-microsoft-edge-developer/suggestions/6508820-javascript-browser-extensions'>does
not yet support extensions</a>. However, you can be sure that a version of
Adblock Plus for Microsoft Edge will be released as soon as possible. Enter your
email in the field above to receive a notification when Adblock Plus for
Microsoft Edge is available. While you're waiting, you may want to try the
latest version of Adblock Plus for Internet
Explorer"|translate("ms-edge-description")}}
On 2015/08/20 20:54:53, saroyanm wrote:
> I don't remember this text to be specified. Also missing period missing. Would
> be great if you could check the text back with Lisa.

This has now been approved by Lisa. See here:
https://docs.google.com/document/d/10FyTjdghZw6dVYvVWegaElAwB-xfxuQACm5P6ay0_...

https://codereview.adblockplus.org/29322769/diff/29323520/static/css/index-de...
File static/css/index-desktop.css (right):

https://codereview.adblockplus.org/29322769/diff/29323520/static/css/index-de...
static/css/index-desktop.css:65: margin: 0px 35px;
On 2015/08/20 20:54:54, saroyanm wrote:
> Looking to visual seems like we have top margin here.

Not sure what you mean here. The element positioning is absolute here.

https://codereview.adblockplus.org/29322769/diff/29323520/static/css/index-de...
static/css/index-desktop.css:91: margin-top: 40px;
On 2015/08/20 20:54:54, saroyanm wrote:
> Why this have been changed ?

It was to accommodate the style guide. But I don't think that's really needed.
Reverted.

saroyanm

https://codereview.adblockplus.org/29322769/diff/29328465/includes/index.tmpl File includes/index.tmpl (right): https://codereview.adblockplus.org/29322769/diff/29328465/includes/index.tmpl#newcode68 includes/index.tmpl:68: <li id="feature-edge" itemprop="featureList"> It's not part of the feature ...

Sept. 22, 2015, 1:15 p.m. (2015-09-22 13:15:47 UTC) #6

Oleksandr

Sept. 23, 2015, 12:37 a.m. (2015-09-23 00:37:58 UTC) #7

https://codereview.adblockplus.org/29322769/diff/29328465/includes/index.tmpl
File includes/index.tmpl (right):

https://codereview.adblockplus.org/29322769/diff/29328465/includes/index.tmpl...
includes/index.tmpl:68: <li id="feature-edge" itemprop="featureList">
On 2015/09/22 13:15:45, saroyanm wrote:
> It's not part of the feature block, so please move it to it's own block
between
> "features" and "install" block, you can give it more specific ID and access
> Form, Button and textbox through that block. Ex.: edge-subscription.
> So you will access the mentioned elements like: "#edge-subscription form",
> "#edge-subscription button" (or if you want to be more specific than you can
use
> "#edge-subscription button[type=submit]") and "#edge-subscription input".

I agree that it makes sense to move this away from the UL, however I don't see
how would getting rid of the IDs be useful. We used IDs in for the iOS
subscription page, and if we remove them here, there will be a lot of
querySelector calls in JavaScript in index.js, which is slower and less
intuitive, IMO.

https://codereview.adblockplus.org/29322769/diff/29328465/static/css/index.css
File static/css/index.css (right):

https://codereview.adblockplus.org/29322769/diff/29328465/static/css/index.cs...
static/css/index.css:695: height: 36px; /* IE 8 */
On 2015/09/22 13:15:46, saroyanm wrote:
> This should be redundant if we don't expect IE8 users to visit the page.

We still support IE8. It's the oldest IE we support right now.

https://codereview.adblockplus.org/29322769/diff/29328465/static/js/index.js
File static/js/index.js (right):

https://codereview.adblockplus.org/29322769/diff/29328465/static/js/index.js#...
static/js/index.js:3: function addListener(obj, type, listener, useCapture)
On 2015/09/22 13:15:46, saroyanm wrote:
> Should be redundant if we doesn't expect IE8 users to visit the page, same
goes
> to other IE8 related code here.

We still support IE8.

saroyanm

Can you also please fix the issue number, I do not have edit rights ? ...

Sept. 23, 2015, 12:44 p.m. (2015-09-23 12:44:22 UTC) #8

Can you also please fix the issue number, I do not have edit rights ?

https://codereview.adblockplus.org/29322769/diff/29328465/includes/index.tmpl
File includes/index.tmpl (right):

https://codereview.adblockplus.org/29322769/diff/29328465/includes/index.tmpl...
includes/index.tmpl:68: <li id="feature-edge" itemprop="featureList">
My point was about CSS actually, but still.
> I don't see how would getting rid of the IDs be useful.
I can see several reason why I'd prefer not use ID for each element here, beside
that It's not good experience to assign ID to each element IMO: 
1. It will minimize the risk of ID duplication
2. You can reuse the styles for the child elements for future forms on homepage
(well this point is not related, but when you have IDs people intentionally can
try assign styles to IDs)
3. You can reuse the function for future forms on the page, otherwise we will
need to modify current implementation and retest it, which can lead to errors in
future.
> We used IDs in for the iOS
> subscription page, and if we remove them here, there will be a lot of
> querySelector calls in JavaScript in index.js, which is slower and less
> intuitive, IMO.
Yes we used IDs, but the page wasn't so complex, you can use edge block to
related to child elements and use getElementsByTagName and
getElementsByClassName, shouldn't harm the performance that much, beside that
the  script will be executed on submission, the only script that will run before
submission is the placeholder hack for old browsers, so we are covering most
cases IMO, so performance wise we are okey here.

https://codereview.adblockplus.org/29322769/diff/29328465/static/css/index.css
File static/css/index.css (right):

https://codereview.adblockplus.org/29322769/diff/29328465/static/css/index.cs...
static/css/index.css:695: height: 36px; /* IE 8 */
On 2015/09/23 00:37:57, Oleksandr wrote:
> On 2015/09/22 13:15:46, saroyanm wrote:
> > This should be redundant if we don't expect IE8 users to visit the page.
> 
> We still support IE8. It's the oldest IE we support right now.

Ahh fare enough, don't know why I thought we wouldn't expect IE8 users visit the
page, but yes just ignore my IE8 related questions.

https://codereview.adblockplus.org/29322769/diff/29328465/static/js/index.js
File static/js/index.js (right):

https://codereview.adblockplus.org/29322769/diff/29328465/static/js/index.js#...
static/js/index.js:3: function addListener(obj, type, listener, useCapture)
On 2015/09/23 00:37:57, Oleksandr wrote:
> On 2015/09/22 13:15:46, saroyanm wrote:
> > Should be redundant if we doesn't expect IE8 users to visit the page, same
> goes
> > to other IE8 related code here.
> 
> We still support IE8.

Fare enough, just ignore the IE8 related comments.

https://codereview.adblockplus.org/29322769/diff/29328503/static/css/index-mo...
File static/css/index-mobile.css (right):

https://codereview.adblockplus.org/29322769/diff/29328503/static/css/index-mo...
static/css/index-mobile.css:115: #subscribe-textbox
Please don't use calc It's not supported in IE8 which we decided to support and
beside that you have better way to achieve this result:
Please use border-box; to correct width calculation of input box and use width:
100%, so in general (also for button):
webkit-box-sizing: border-box;
-moz-box-sizing: border-box;
-ms-box-sizing: border-box;
-o-box-sizing: border-box;
box-sizing: border-box;
width: 100%;

https://codereview.adblockplus.org/29322769/diff/29328503/static/css/index.css
File static/css/index.css (right):

https://codereview.adblockplus.org/29322769/diff/29328503/static/css/index.cs...
static/css/index.css:667: margin: 0px 0px 10px 40px;
In general I'd be fan of not aligning this with the UL element indentation, but
now I see that in the mockup we are doing so, so in that case we should also
consider RTL direction. 
so please use html[dir="rtl"], to make it consistent with other RTL hacks in the
page.

https://codereview.adblockplus.org/29322769/diff/29328503/static/css/index.cs...
static/css/index.css:679: padding-left: 10px;
Please use padding: 0px 10px; here to make it consistent with other similar
implementations.

https://codereview.adblockplus.org/29322769/diff/29328503/static/css/index.cs...
static/css/index.css:700: min-width: 146px;
Please use width instead of min-width, so we will be sure that the button will
not exceed the limits for different translations.

https://codereview.adblockplus.org/29322769/diff/29328503/static/css/index.cs...
static/css/index.css:705: #subscribe-button span
Please remove this rule, we should also consider, longer text for translations,
also in Firefox the layout is broken, probably because of the font rendering
difference.
IMO in this case it's better for text to wrap.

https://codereview.adblockplus.org/29322769/diff/29328503/static/css/index.cs...
static/css/index.css:713: #subscribe-form.success button
Probably can be merged with rule above (#other-browsers).

Oleksandr

I think all of your comments have been addressed. Javascript looks kind of wonky IMO, ...

Sept. 24, 2015, 2:34 a.m. (2015-09-24 02:34:05 UTC) #9

saroyanm

On 2015/09/24 02:34:05, Oleksandr wrote: > I think all of your comments have been addressed. ...

Sept. 24, 2015, 8:28 a.m. (2015-09-24 08:28:17 UTC) #10

saroyanm

On 2015/09/24 08:28:17, saroyanm wrote: > On 2015/09/24 02:34:05, Oleksandr wrote: > > I think ...

Sept. 24, 2015, 2:51 p.m. (2015-09-24 14:51:31 UTC) #11

saroyanm

https://codereview.adblockplus.org/29322769/diff/29328556/includes/index.tmpl File includes/index.tmpl (right): https://codereview.adblockplus.org/29322769/diff/29328556/includes/index.tmpl#newcode75 includes/index.tmpl:75: <form accept-charset="utf-8" action="/submitEmail" method="post"> Can you please hardcode the ...

Sept. 24, 2015, 2:52 p.m. (2015-09-24 14:52:06 UTC) #12

Oleksandr

https://codereview.adblockplus.org/29322769/diff/29328556/static/js/index.js File static/js/index.js (right): https://codereview.adblockplus.org/29322769/diff/29328556/static/js/index.js#newcode87 static/js/index.js:87: params += "&lang=" + pathArray[1]; On 2015/09/24 14:52:04, saroyanm ...

Sept. 25, 2015, 12:35 a.m. (2015-09-25 00:35:49 UTC) #13

saroyanm

I think we almost there, I think soon the code will be reusable for other ...

Sept. 29, 2015, 9:58 a.m. (2015-09-29 09:58:42 UTC) #14

I think we almost there, I think soon the code will be reusable for other
similar announcements as well, more easily.

Also can you please CC Felix while he is super reviewer of websites module.

https://codereview.adblockplus.org/29322769/diff/29328556/static/js/index.js
File static/js/index.js (right):

https://codereview.adblockplus.org/29322769/diff/29328556/static/js/index.js#...
static/js/index.js:87: params += "&lang=" + pathArray[1];
On 2015/09/25 00:35:45, Oleksandr wrote:
> On 2015/09/24 14:52:04, saroyanm wrote:
> > Please note that this will return empty string for users who will visit the
> > website with MS Edge, because with our setup the language and plaform url is
> not
> > being show for users visiting http://adblockplus.org, so probably we can't
> determine
> > the language that way, I don't have strong opinion on this, but maybe we can
> get
> > the locale using:
> > document.documentElement.lang
> 
> I don't have a strong opinion on this either, but this parameter is ignored
> anyway. So I think it is fine as is.

IMO make sense to remove it or provide correct implementation in case in future
we will provide email dependent on page locale, commented in your new patch
regarding updating this implementation.

https://codereview.adblockplus.org/29322769/diff/29328556/static/js/index.js#...
static/js/index.js:88: params += "&product=" + "edge";
On 2015/09/25 00:35:45, Oleksandr wrote:
> On 2015/09/24 14:52:05, saroyanm wrote:
> > Where is this coming from ? I can't find reference to this new parameter in
> any
> > related ticket.
> 
> https://hg.adblockplus.org/sitescripts/rev/19391de96062#l3.30

Thanks for the reference, Updated the ticket accordingly.

https://codereview.adblockplus.org/29322769/diff/29328636/includes/index.tmpl
File includes/index.tmpl (right):

https://codereview.adblockplus.org/29322769/diff/29328636/includes/index.tmpl...
includes/index.tmpl:76: <input class="subscribe-textbox" type="email"
name="email" placeholder="{{"Your email
address"|translate("subscription-input-placeholder")}}" required />
I think we need to add hidden inputs here that will keep the value of page an
locale, so we will need to add this inputs as well IMO:
<input type="hidden" name="lang" value="{{locale}}" />
<input type="hidden" name="product" value="{{page}}" />

https://codereview.adblockplus.org/29322769/diff/29328636/includes/index.tmpl...
includes/index.tmpl:77: <button class="subscribe-button"
type="submit"><span>{{"Notify
me"|translate("subscription-button")}}</span></button>
The class I think is redundant, doesn't look like we use.

https://codereview.adblockplus.org/29322769/diff/29328636/static/js/index.js
File static/js/index.js (right):

https://codereview.adblockplus.org/29322769/diff/29328636/static/js/index.js#...
static/js/index.js:46: function contentLoad()
Please rename the function, in general the content is not yet fully loaded at
this point, so I'd rename the function to something more descriptive ex.:
initSubscriptionForm(edgeSubscription), also please pass the form element as
parameter here ex.:

initSubscriptionForm(document.getElementById("edge-subscription"));

https://codereview.adblockplus.org/29322769/diff/29328636/static/js/index.js#...
static/js/index.js:48: var edgeSubscription =
document.getElementById("edge-subscription");
This will be redundant after the suggested implementation above.

https://codereview.adblockplus.org/29322769/diff/29328636/static/js/index.js#...
static/js/index.js:49: var emailTextbox =
edgeSubscription.getElementsByClassName("subscribe-textbox")[0];
In general I'd be fan of using queryselector because it should harm the
performance that much and will make the code more clear IMO, but I'll let it up
to you to keep it like this or change to queryselectors while old code on
homepage looks terrible, because of migration and old support reuqirements.

https://codereview.adblockplus.org/29322769/diff/29328636/static/js/index.js#...
static/js/index.js:72: var subscriptionForm =
document.getElementById("edge-subscription").
You have the "edge-subscription" element already, no need to get it from
document again.

https://codereview.adblockplus.org/29322769/diff/29328636/static/js/index.js#...
static/js/index.js:76: var formElement = subscriptionForm;
I think this assignment is redundant.

https://codereview.adblockplus.org/29322769/diff/29328636/static/js/index.js#...
static/js/index.js:84: var params = emailTextbox.name + "=" +
encodeURIComponent(emailTextbox.value);
After implementing the changes in index.tmpl you can get parameters from input
boxes ex.:
var inputs = subscriptionForm.getElementsByTagName("input");
var params = "";
for (var i = 0; i < inputs.length; i++)
{
  if (params)
    params += "&";
  params += inputs[i].name + "=" + encodeURIComponent(inputs[i].value);
}

Oleksandr

https://codereview.adblockplus.org/29322769/diff/29328636/static/js/index.js File static/js/index.js (right): https://codereview.adblockplus.org/29322769/diff/29328636/static/js/index.js#newcode49 static/js/index.js:49: var emailTextbox = edgeSubscription.getElementsByClassName("subscribe-textbox")[0]; On 2015/09/29 09:58:40, saroyanm wrote: ...

Sept. 30, 2015, 8:17 a.m. (2015-09-30 08:17:04 UTC) #15

saroyanm

Almost there. @Ollie can you also be sure that the images in this review also ...

Sept. 30, 2015, 12:33 p.m. (2015-09-30 12:33:05 UTC) #16

Oleksandr

Done. I hope I understood your intentions correctly. The edge-teaser was already compressed in patchset ...

Sept. 30, 2015, 2:36 p.m. (2015-09-30 14:36:04 UTC) #17

saroyanm

Just noticed a compatibility issue, but should be easy to fix. https://codereview.adblockplus.org/29322769/diff/29328722/includes/index.tmpl File includes/index.tmpl (right): ...

Sept. 30, 2015, 3:38 p.m. (2015-09-30 15:38:40 UTC) #18

Oleksandr

Patch set 12! 'nuff said :) https://codereview.adblockplus.org/29322769/diff/29328722/includes/index.tmpl File includes/index.tmpl (right): https://codereview.adblockplus.org/29322769/diff/29328722/includes/index.tmpl#newcode151 includes/index.tmpl:151: <img id="edge-teaser" src="/img/edge_teaser.png"> ...

Sept. 30, 2015, 7:57 p.m. (2015-09-30 19:57:28 UTC) #19

saroyanm

Oct. 1, 2015, 10:49 a.m. (2015-10-01 10:49:31 UTC) #20

On 2015/09/30 19:57:28, Oleksandr wrote:
> Patch set 12! 'nuff said :)
> 
> https://codereview.adblockplus.org/29322769/diff/29328722/includes/index.tmpl
> File includes/index.tmpl (right):
> 
>
https://codereview.adblockplus.org/29322769/diff/29328722/includes/index.tmpl...
> includes/index.tmpl:151: <img id="edge-teaser" src="/img/edge_teaser.png">
> On 2015/09/30 15:38:38, saroyanm wrote:
> > Detail: Please also add alt required attribute to img tag and close the tag
> ex.:
> > <img id="edge-teaser" src="/img/edge_teaser.png" alt="{{"Comming soon image
of
> > Adblock Plus for Microsoft Edge"|translate("edge-teaser")}}" />
> 
> The maxthon image above also needs this fix, but I think it would be better
done
> in a separate patch set.

LGTM

Maybe it will make sense to do a cosmetic change afterward, IMO the Error
message and Success message doesn't look nice, the success message also not
noticeable.

Expand All Messages | Collapse All Messages