index.html - Issue 29646555: Issue 6210 - Implement Subscription (Double-opt-in) template for newsletter

Side by Side Diff: index.html

Issue 29646555: Issue 6210 - Implement Subscription (Double-opt-in) template for newsletter (Closed)

Patch Set: Update hgignore Created Dec. 21, 2017, 11:50 a.m.

Left:
Right:

Use n/p to move between diff chunks; N/P to move between comments.

Jump to:

View unified diff | Download patch

OLD	NEW
(Empty)
	1 <!doctype html>

	2 <html lang="en" dir="ltr">
	saroyanm 2017/12/21 16:07:58 I assume we only planing to have English version r I assume we only planing to have English version right ? As I couldn't find that information in the Specification. juliandoucette 2017/12/21 22:06:51 (I think the implication here is that we don't nee (I think the implication here is that we don't need lang or dir) ire 2017/12/22 10:33:25 I also believe that we're only having an English v Show quoted text On 2017/12/21 16:07:58, saroyanm wrote: > I assume we only planing to have English version right ? > As I couldn't find that information in the Specification. I also believe that we're only having an English version. We may not need the "dir", but the "lang" attribute needs to be there regardless. I also think we should have the "dir" attribute as well, but it's not required like the lang attribute is for page validity juliandoucette 2017/12/22 13:32:22 In that case I agree that we should keep both of t Show quoted text On 2017/12/22 10:33:25, ire wrote: > I also believe that we're only having an English version. > > We may not need the "dir", but the "lang" attribute needs to be there > regardless. I also think we should have the "dir" attribute as well, but it's > not required like the lang attribute is for page validity In that case I agree that we should keep both of them.
	3 <head>

	4 <meta charset="utf-8">
	saroyanm 2017/12/21 16:07:58 There is also a meta description provided in the s There is also a meta description provided in the specs "A newsletter sign up form, so users can stay up to date with all things related to Adblock Plus", which is missing here, but I assume it might be still be relevant how backClick is implemented. ire 2017/12/22 10:33:25 It should be here as well, I will add that now. Show quoted text On 2017/12/21 16:07:58, saroyanm wrote: > There is also a meta description provided in the specs "A newsletter sign up > form, so users can stay up to date with all things related to Adblock Plus", > which is missing here, but I assume it might be still be relevant how backClick > is implemented. It should be here as well, I will add that now. ire 2017/12/22 10:33:25 Done. Show quoted text On 2017/12/21 16:07:58, saroyanm wrote: > There is also a meta description provided in the specs "A newsletter sign up > form, so users can stay up to date with all things related to Adblock Plus", > which is missing here, but I assume it might be still be relevant how backClick > is implemented. Done.
	5 <title>Adblock Plus newsletter signup form</title>
	juliandoucette 2017/12/21 22:06:51 Also I think this should be "Newsletter Signup \| A Also I think this should be "Newsletter Signup \| Adblock Plus" - To be consistent with other website titles - Because this is more "Newsletter Signup" (primary keyword) than "Adblock Plus" (secondary keyword / brand) - Because "form" is useless in this title? - Meaning that "signup" implies "form" and "form" is not a very searchable keyword juliandoucette 2017/12/21 22:06:51 Do we know whether or not search engines and socia Do we know whether or not search engines and social media scrapers will index this page? If so, we should consider other meta data too. I would propose that we support the website-default standard meta data for the first release and then extend it with social media meta data separately. But we may have to rush social media meta data if the comms team wants to advertise this right away on social media. ire 2017/12/22 10:33:23 I don't know about this. If so, we should conside Show quoted text On 2017/12/21 22:06:51, juliandoucette wrote: > Do we know whether or not search engines and social media scrapers will index > this page? I don't know about this. If so, we should consider other meta data too. I would propose that Show quoted text > we support the website-default standard meta data for the first release and then > extend it with social media meta data separately. But we may have to rush social > media meta data if the comms team wants to advertise this right away on social > media. Done. ire 2017/12/22 10:33:23 Could you make a comment in the Specification abou Show quoted text On 2017/12/21 22:06:51, juliandoucette wrote: > Also I think this should be "Newsletter Signup \| Adblock Plus" > > - To be consistent with other website titles > - Because this is more "Newsletter Signup" (primary keyword) than "Adblock Plus" > (secondary keyword / brand) > - Because "form" is useless in this title? > - Meaning that "signup" implies "form" and "form" is not a very searchable > keyword Could you make a comment in the Specification about this? I would rather leave it the way it is specified first, and we can always make this change when it's implemented in the spec. juliandoucette 2017/12/22 13:32:22 Yes. Show quoted text On 2017/12/22 10:33:23, ire wrote: > Could you make a comment in the Specification about this? I would rather leave > it the way it is specified first, and we can always make this change when it's > implemented in the spec. Yes.
	6 <meta name="viewport" content="width=device-width, initial-scale=1">

	7 <link rel="stylesheet" type="text/css" href="css/main.min.css">
	juliandoucette 2017/12/21 22:06:50 [Separate, minified] css seems like overkill here. [Separate, minified] css seems like overkill here. Perhaps we can use our build process to insert uncompressed CSS into this page? (uncompressed because sourcemaps are overkill?) ire 2017/12/22 10:33:25 (I mentioned in a separate comment) that my plan i Show quoted text On 2017/12/21 22:06:50, juliandoucette wrote: > [Separate, minified] css seems like overkill here. Perhaps we can use our build > process to insert uncompressed CSS into this page? (uncompressed because > sourcemaps are overkill?) (I mentioned in a separate comment) that my plan is to insert minified CSS inline here. But I also raised the question of whether we need to have licence and if it's important that the CSS is compressed (without source maps) for this particular case. I don't really like the idea of uncompressed, inline CSS here (performance reasons, mainly). Maybe a good middle-ground would be to inline the compressed CSS, and place a link to a full file hosted elsewhere? (Also I haven't figure out how/if we can host files on Backclick which is why I'm favouring inline CSS in the first place) juliandoucette 2017/12/22 13:32:22 I don't think that this little amount of CSS will Show quoted text On 2017/12/22 10:33:25, ire wrote: > On 2017/12/21 22:06:50, juliandoucette wrote: > > [Separate, minified] css seems like overkill here. Perhaps we can use our > build > > process to insert uncompressed CSS into this page? (uncompressed because > > sourcemaps are overkill?) > > (I mentioned in a separate comment) that my plan is to insert minified CSS > inline here. But I also raised the question of whether we need to have licence > and if it's important that the CSS is compressed (without source maps) for this > particular case. I don't really like the idea of uncompressed, inline CSS here > (performance reasons, mainly). Maybe a good middle-ground would be to inline the > compressed CSS, and place a link to a full file hosted elsewhere? I don't think that this little amount of CSS will have a significant impact on performance. Show quoted text > (Also I haven't figure out how/if we can host files on Backclick which is why > I'm favouring inline CSS in the first place) Ack. I don't think it's necessary to include this little amount of CSS separately anyway.
	8 </head>

	9 <body>

	10 <div class="outer-container">

	11 <div class="container phablet-width content">

	12

	13 <header id="header">

	14 <h1 id="title">
	juliandoucette 2017/12/21 22:06:51 "title" has a specific meaning in HTM. Did you cho "title" has a specific meaning in HTM. Did you choose "title" for a platform specific reason? (The same applies for header#header. If not, I would suggest header#brand and the selectors #brand and #brand h1.) ire 2017/12/22 10:33:23 I chose #title and #header simply because its esse Show quoted text On 2017/12/21 22:06:51, juliandoucette wrote: > "title" has a specific meaning in HTM. Did you choose "title" for a platform > specific reason? > > (The same applies for header#header. If not, I would suggest header#brand and > the selectors #brand and #brand h1.) I chose #title and #header simply because its essentially a one-page site and there is only one title and header. I don't think #brand is necessarily correct because there is other text in the header that is not "brand". Would you prefer #page-header ? juliandoucette 2017/12/22 13:32:22 Ack. Yes. Show quoted text On 2017/12/22 10:33:23, ire wrote: > I chose #title and #header simply because its essentially a one-page site and > there is only one title and header. I don't think #brand is necessarily correct > because there is other text in the header that is not "brand". Would you prefer > #page-header ? Ack. Yes.
	15 <img src="img/abp-logo.png" srcset="img/abp-logo.svg 2x" alt="ABP insi de red octagon">

	16 <span>Adblock<strong>Plus</strong></span>
	saroyanm 2017/12/21 16:07:58 Suggestion/detail: we might want to separate the w Suggestion/detail: we might want to separate the words, I think we had same question in acceptableads.org header's Acceptable Ads text, which visually one word, but semantically separate. Same or similar Technic should work here as well. juliandoucette 2017/12/21 22:06:51 I think we should follow in adblockplus.org's exam Show quoted text On 2017/12/21 16:07:58, saroyanm wrote: > Suggestion/detail: we might want to separate the words, I think we had same > question in http://acceptableads.org header's Acceptable Ads text, which visually one > word, but semantically separate. > > Same or similar Technic should work here as well. I think we should follow in adblockplus.org's example and separate these words for the sake of consistency. ire 2017/12/22 10:33:23 Done. Show quoted text On 2017/12/21 16:07:58, saroyanm wrote: > Suggestion/detail: we might want to separate the words, I think we had same > question in http://acceptableads.org header's Acceptable Ads text, which visually one > word, but semantically separate. > > Same or similar Technic should work here as well. Done. ire 2017/12/22 10:33:25 Done. Show quoted text On 2017/12/21 22:06:51, juliandoucette wrote: > On 2017/12/21 16:07:58, saroyanm wrote: > > Suggestion/detail: we might want to separate the words, I think we had same > > question in http://acceptableads.org header's Acceptable Ads text, which > visually one > > word, but semantically separate. > > > > Same or similar Technic should work here as well. > > I think we should follow in adblockplus.org's example and separate these words > for the sake of consistency. Done.
	17 </h1>

	18 <p class="lead"><strong>Sign up for the Adblock Plus newsletter</strong> </p>
	saroyanm 2017/12/21 16:07:58 Detail: According to the specs it's a Heading, not Detail: According to the specs it's a Heading, not paragraph. Detail: Also this looks to be a bit smaller in the specs image. Maybe "1.1em" will fit right, anyway hard to tell as there is no styleguide. juliandoucette 2017/12/21 22:06:51 I kinda see where she's coming from... (I've gone Show quoted text On 2017/12/21 16:07:58, saroyanm wrote: > Detail: According to the specs it's a Heading, not paragraph. I kinda see where she's coming from... (I've gone back and forth a few times while writing this reply). But I think that "Sign up for the Adblock Plus newsletter" makes a better h1 than "Adblock Plus" on this page because this page is primarily about signing up for the news letter; not Adblock Plus. Therefore, I would suggest something like this: <header id="brand"> <figure> <img src="img/abp-logo.png" srcset="img/abp-logo.svg 2x" alt="ABP inside red octagon"> <figcaption>Adblock <strong>Plus</strong></figcaption> </figure> <h1 class="h5">Sign up for the Adblock Plus newsletter</h1> <p>Stay up to date with all things Adblock Plus</p> </header> ire 2017/12/22 10:33:24 Done. Show quoted text On 2017/12/21 22:06:51, juliandoucette wrote: > On 2017/12/21 16:07:58, saroyanm wrote: > > Detail: According to the specs it's a Heading, not paragraph. > > I kinda see where she's coming from... (I've gone back and forth a few times > while writing this reply). > > But I think that "Sign up for the Adblock Plus newsletter" makes a better h1 > than "Adblock Plus" on this page because this page is primarily about signing up > for the news letter; not Adblock Plus. Therefore, I would suggest something like > this: > > <header id="brand"> > <figure> > <img src="img/abp-logo.png" srcset="img/abp-logo.svg 2x" alt="ABP inside red > octagon"> > <figcaption>Adblock <strong>Plus</strong></figcaption> > </figure> > <h1 class="h5">Sign up for the Adblock Plus newsletter</h1> > <p>Stay up to date with all things Adblock Plus</p> > </header> Done. ire 2017/12/22 10:33:24 Done. Show quoted text On 2017/12/21 16:07:58, saroyanm wrote: > Detail: According to the specs it's a Heading, not paragraph. > > Detail: Also this looks to be a bit smaller in the specs image. Maybe "1.1em" > will fit right, anyway hard to tell as there is no styleguide. Done.
	19 <p>Stay up to date with all things Adblock Plus</p>

	20 </header>

	21

	22 <form class="phone-width align-center">
	saroyanm 2017/12/21 16:07:57 I'll sugget to assign styles to the form itself ra I'll sugget to assign styles to the form itself rather than maintain two different classes separately. juliandoucette 2017/12/21 22:06:51 NIT/TOL: The use of these device widths seems to b NIT/TOL: The use of these device widths seems to be a little contrived to me :/ (Meaning: I don't think that the page was designed to be these device widths; they just ~conveniently fit.) ire 2017/12/22 10:33:24 I'm not sure I understand what you mean. Do you th Show quoted text On 2017/12/21 22:06:51, juliandoucette wrote: > NIT/TOL: The use of these device widths seems to be a little contrived to me :/ > > (Meaning: I don't think that the page was designed to be these device widths; > they just ~conveniently fit.) I'm not sure I understand what you mean. Do you think the widths I'm using are not suitable? ire 2017/12/22 10:33:24 Done. Show quoted text On 2017/12/21 16:07:57, saroyanm wrote: > I'll sugget to assign styles to the form itself rather than maintain two > different classes separately. Done.
	23 <label for="email">Email Address *</label>

	24 <input type="email" id="email" name="email" required>

	25 <span class="error-message">Please enter a valid email address</span>
	saroyanm 2017/12/21 16:07:58 Specs need clarification: it's not obvious whether Specs need clarification: it's not obvious whether this validation message appears on submit or during the typing. saroyanm 2017/12/21 16:07:58 I assume you would like to figure out first how va I assume you would like to figure out first how validation is done in Backclick before tackling this, but anyway I add this as a note: Currently custom validation message(as in the specs) is not being shown on validation. ire 2017/12/22 10:33:22 Yes I will be using Backclick's method for validat Show quoted text On 2017/12/21 16:07:58, saroyanm wrote: > I assume you would like to figure out first how validation is done in Backclick > before tackling this, but anyway I add this as a note: > Currently custom validation message(as in the specs) is not being shown on > validation. Yes I will be using Backclick's method for validation. ire 2017/12/22 10:33:26 I think according to the spec, it should happen be Show quoted text On 2017/12/21 16:07:58, saroyanm wrote: > Specs need clarification: it's not obvious whether this validation message > appears on submit or during the typing. I think according to the spec, it should happen before submit. However, Backclick has a way to show this after submit. Because of time, I would rather use the Backclick method (which will add the class after submit to show the error message). This is just quicker for me to do instead of having to validate manually. I think we can make this improvement later on. saroyanm 2017/12/22 12:26:11 I agree. Show quoted text On 2017/12/22 10:33:26, ire wrote: > On 2017/12/21 16:07:58, saroyanm wrote: > > Specs need clarification: it's not obvious whether this validation message > > appears on submit or during the typing. > > I think according to the spec, it should happen before submit. However, > Backclick has a way to show this after submit. Because of time, I would rather > use the Backclick method (which will add the class after submit to show the > error message). This is just quicker for me to do instead of having to validate > manually. I think we can make this improvement later on. I agree. juliandoucette 2017/12/22 13:32:23 Please point this out in the spec. Show quoted text On 2017/12/22 10:33:26, ire wrote: > I think according to the spec, it should happen before submit. However, > Backclick has a way to show this after submit. Because of time, I would rather > use the Backclick method (which will add the class after submit to show the > error message). This is just quicker for me to do instead of having to validate > manually. I think we can make this improvement later on. Please point this out in the spec.
	26

	27 <label for="fullname">Full name</label>

	28 <input type="text" id="fullname" name="fullname">

	29

	30 <button type="submit" class="button secondary">Sign Up</button>
	saroyanm 2017/12/21 16:07:58 Might be irrelevant while you check how backclick Might be irrelevant while you check how backclick works, but will leave here just in case: According to specs, the button is inactive until the field is valid. ire 2017/12/22 10:33:25 Yes. As I mentioned in my comment above, in order Show quoted text On 2017/12/21 16:07:58, saroyanm wrote: > Might be irrelevant while you check how backclick works, but will leave here > just in case: > According to specs, the button is inactive until the field is valid. Yes. As I mentioned in my comment above, in order to use Backclick's method, the validation will only show after submit. juliandoucette 2017/12/22 13:32:22 Again, please point this out in the spec. Show quoted text On 2017/12/22 10:33:25, ire wrote: > Yes. As I mentioned in my comment above, in order to use Backclick's method, the > validation will only show after submit. Again, please point this out in the spec.
	31

	32 <p class="text-center">
	saroyanm 2017/12/21 16:07:58 Detail: According to the specification the text is Detail: According to the specification the text is aligned to the left on the "desktop" view. juliandoucette 2017/12/21 22:06:50 NIT: I think that this should be inside a <footer> NIT: I think that this should be inside a <footer> ire 2017/12/22 10:33:24 Done. Show quoted text On 2017/12/21 16:07:58, saroyanm wrote: > Detail: According to the specification the text is aligned to the left on the > "desktop" view. Done. ire 2017/12/22 10:33:25 Done. Show quoted text On 2017/12/21 22:06:50, juliandoucette wrote: > NIT: I think that this should be inside a <footer> Done.
	33 <a href="https://adblockplus.org/en/privacy" target="_blank">Privacy P olicy</a>
	saroyanm 2017/12/21 16:07:57 I assume we are planing to update the privacy poli I assume we are planing to update the privacy policy to reflect the backclick privacy policy and link to that header, right ? Also, make sense to check to which website the update will land, because we also used to have extended Privacy Policy in eyeo.com, with Privacy Policy information like "talent tracker". ire 2017/12/22 10:33:25 I'm not sure about this. I will post this comment Show quoted text On 2017/12/21 16:07:57, saroyanm wrote: > I assume we are planing to update the privacy policy to reflect the backclick > privacy policy and link to that header, right ? > Also, make sense to check to which website the update will land, because we also > used to have extended Privacy Policy in http://eyeo.com, with Privacy Policy > information like "talent tracker". I'm not sure about this. I will post this comment to Trac to ask Jeen/Winsley ire 2017/12/22 11:46:28 See comment here https://issues.adblockplus.org/ti Show quoted text On 2017/12/21 16:07:57, saroyanm wrote: > I assume we are planing to update the privacy policy to reflect the backclick > privacy policy and link to that header, right ? > Also, make sense to check to which website the update will land, because we also > used to have extended Privacy Policy in http://eyeo.com, with Privacy Policy > information like "talent tracker". See comment here https://issues.adblockplus.org/ticket/6210#comment:10 I have changed the link to the eyeo.com Privacy Policy page juliandoucette 2017/12/22 13:32:22 Acknowledged. Show quoted text On 2017/12/22 11:46:28, ire wrote: > On 2017/12/21 16:07:57, saroyanm wrote: > > I assume we are planing to update the privacy policy to reflect the backclick > > privacy policy and link to that header, right ? > > Also, make sense to check to which website the update will land, because we > also > > used to have extended Privacy Policy in http://eyeo.com, with Privacy Policy > > information like "talent tracker". > > See comment here https://issues.adblockplus.org/ticket/6210#comment:10 > > I have changed the link to the http://eyeo.com Privacy Policy page Acknowledged.
	34 </p>

	35 </form>

	36 </div>

	37 </div>

	38 </body>

	39 </html>

OLD	NEW

« no previous file with comments | « img/abp-logo.svg ('k') | package.json » ('j') | scss/_content.scss » ('J')