Issue 29646555: Issue 6210 - Implement Subscription (Double-opt-in) template for newsletter (Closed)
DescriptionIssue
: https://gitlab.com/eyeo/web.adblockplus.org/issues/1
Specification
: https://gitlab.com/eyeo/spec/merge_requests/110
## Relevant documentation
Subscription template
: https://www.backclick.de/confluence/display/BC5EN/Subscription+template
Subscription (Double-opt-in)
: https://www.backclick.de/confluence/pages/viewpage.action?pageId=4751543
Database designer
: https://www.backclick.de/confluence/display/BC5EN/The+database+designer
COLLABORATOR=julian@adblockplus.org
Patch Set 1 #
Total comments: 7
Patch Set 2 : Update hgignore #
Total comments: 76
Patch Set 3 : Addressed comments #4 and #5 #Patch Set 4 : Add comments to Backclick template #Patch Set 5 : Update privacy policy link #
Total comments: 23
Patch Set 6 : Addressed comments #Patch Set 7 : Update sender email #Patch Set 8 : Testing backclick template changes #Patch Set 9 : Add gulp task to inline css, update templates #Patch Set 10 : Update confirmation and success texts #
Total comments: 43
Patch Set 11 : Addressed comments #15 #Patch Set 12 : Remove eRobinson feature #Patch Set 13 : Update with specification changes #
Total comments: 31
Patch Set 14 : Addressed comments #20 #
MessagesTotal messages: 22
@Manvel please take a look at this when you can. I've added some background information to the review description, but let me know if anything is unclear. Thank you!
https://codereview.adblockplus.org/29646555/diff/29646556/index.html File index.html (right): https://codereview.adblockplus.org/29646555/diff/29646556/index.html#newcode7 index.html:7: <link rel="stylesheet" type="text/css" href="css/main.min.css"> In the actual implementation, I will likely inline these styles because: - I'm not sure how to host files with Backclick - The stylesheet isn't that big so we might as well not have an additional resource to have to fetch However, I'm not sure if: 1. We can inline the minified file (no sourcemaps) 2. We need to have a licence My suggestion is, if you don't have an answer right now, that we inline the minified file and update later if necessary https://codereview.adblockplus.org/29646555/diff/29646556/index.html#newcode15 index.html:15: <img src="img/abp-logo.png" srcset="img/abp-logo.svg 2x" alt="ABP inside red octagon"> I used this logo instead of the full one (with text) because I think it looks better to have the "AdblockPlus" in actual text and because this is the way it's done in the extension UI. https://codereview.adblockplus.org/29646555/diff/29646556/index.html#newcode24 index.html:24: <input type="email" id="email" name="email" required> You can demo what it would look like if this field is incorrect by adding the class, "input_round_error" to it
Comments are ready. Some of my comments may or may not be relevant, because as you mentioned we still not sure how BackClick works, so I marked them as so. Also some of other comments might be website-default specific I also mentioned/marked them as so in the comments I think. Hope they are helpful. https://codereview.adblockplus.org/29646555/diff/29646556/index.html File index.html (right): https://codereview.adblockplus.org/29646555/diff/29646556/index.html#newcode7 index.html:7: <link rel="stylesheet" type="text/css" href="css/main.min.css"> On 2017/12/21 11:54:05, ire wrote: > In the actual implementation, I will likely inline these styles because: > - I'm not sure how to host files with Backclick Do they have an API documentation in English ? They site seems to be only in German. > - The stylesheet isn't that big so we might as well not have an additional > resource to have to fetch This is a relatively simple page, we might be able to have a simple Internal CSS without website-defaults, but I'll leave it up to you and Julian to decide if having whole websites defaults minified here is necessary or not(it doesn't look like that to me though). Also that might mean that we will have Internal Scripts, depending on how we would like to handle validations. > However, I'm not sure if: > 1. We can inline the minified file (no sourcemaps) > 2. We need to have a licence Having HTML header, should be sufficient. > > My suggestion is, if you don't have an answer right now, that we inline the > minified file and update later if necessary https://codereview.adblockplus.org/29646555/diff/29646556/index.html#newcode24 index.html:24: <input type="email" id="email" name="email" required> On 2017/12/21 11:54:05, ire wrote: > You can demo what it would look like if this field is incorrect by adding the > class, "input_round_error" to it Acknowledged. https://codereview.adblockplus.org/29646555/diff/29646566/index.html File index.html (right): https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode2 index.html:2: <html lang="en" dir="ltr"> I assume we only planing to have English version right ? As I couldn't find that information in the Specification. https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode4 index.html:4: <meta charset="utf-8"> 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. https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode16 index.html:16: <span>Adblock<strong>Plus</strong></span> 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. https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode18 index.html:18: <p class="lead"><strong>Sign up for the Adblock Plus newsletter</strong></p> 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. https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode22 index.html:22: <form class="phone-width align-center"> I'll sugget to assign styles to the form itself rather than maintain two different classes separately. https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode25 index.html:25: <span class="error-message">Please enter a valid email address</span> Specs need clarification: it's not obvious whether this validation message appears on submit or during the typing. https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode25 index.html:25: <span class="error-message">Please enter a valid email address</span> 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. https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode30 index.html:30: <button type="submit" class="button secondary">Sign Up</button> 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. https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode32 index.html:32: <p class="text-center"> Detail: According to the specification the text is aligned to the left on the "desktop" view. https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode33 index.html:33: <a href="https://adblockplus.org/en/privacy" target="_blank">Privacy Policy</a> 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". https://codereview.adblockplus.org/29646555/diff/29646566/scss/_form.scss File scss/_form.scss (right): https://codereview.adblockplus.org/29646555/diff/29646566/scss/_form.scss#new... scss/_form.scss:26: .input_round_error Suggestion: What about calling the classname .invalid or error? Doesn't look like that input_round_error inherit any style, until it's a websites module naming convention you are following using underscores. https://codereview.adblockplus.org/29646555/diff/29646566/scss/_form.scss#new... scss/_form.scss:47: .button Why are we introducing new button classes, for the buttons ? I'll suggest to not introduce them, if there is no reason for that(ex: style not button element to look like a button) https://codereview.adblockplus.org/29646555/diff/29646566/scss/_layout.scss File scss/_layout.scss (right): https://codereview.adblockplus.org/29646555/diff/29646566/scss/_layout.scss#n... scss/_layout.scss:20: .align-center Detail: this is only used on the form element, so maybe we can apply style directly to the form. https://codereview.adblockplus.org/29646555/diff/29646566/scss/_vars.scss File scss/_vars.scss (right): https://codereview.adblockplus.org/29646555/diff/29646566/scss/_vars.scss#new... scss/_vars.scss:23: $phone-width: 320px; Seems like we are only using this variable to set the width of the form(until I'm missing smth.), I'll suggest to use different variable for the purpose, ex: form-width.
CC wspee @ire Here are my first impressions. @wspee saroyanm already added his first impressions. https://codereview.adblockplus.org/29646555/diff/29646566/index.html File index.html (right): https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode2 index.html:2: <html lang="en" dir="ltr"> (I think the implication here is that we don't need lang or dir) https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode5 index.html:5: <title>Adblock Plus newsletter signup form</title> 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. https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode5 index.html:5: <title>Adblock Plus newsletter signup form</title> 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 https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode7 index.html:7: <link rel="stylesheet" type="text/css" href="css/main.min.css"> [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?) https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode14 index.html:14: <h1 id="title"> "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.) https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode16 index.html:16: <span>Adblock<strong>Plus</strong></span> 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. https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode18 index.html:18: <p class="lead"><strong>Sign up for the Adblock Plus newsletter</strong></p> 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> https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode22 index.html:22: <form class="phone-width align-center"> 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.) https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode32 index.html:32: <p class="text-center"> NIT: I think that this should be inside a <footer> https://codereview.adblockplus.org/29646555/diff/29646566/scss/_content.scss File scss/_content.scss (right): https://codereview.adblockplus.org/29646555/diff/29646566/scss/_content.scss#... scss/_content.scss:5: a NIT: should be `.content a` if you are trying to be consistent with website-defaults? https://codereview.adblockplus.org/29646555/diff/29646566/scss/_form.scss File scss/_form.scss (right): https://codereview.adblockplus.org/29646555/diff/29646566/scss/_form.scss#new... scss/_form.scss:9: height: 40px; NIT: I'd prefer an em value here https://codereview.adblockplus.org/29646555/diff/29646566/scss/_form.scss#new... scss/_form.scss:10: border-radius: 4px; NIT: I think rounded edges is inconsistent with adblockplus.org (Don't delay publishing as a result, but please do point this out to the product team and ask them to be consistent.) https://codereview.adblockplus.org/29646555/diff/29646566/scss/_form.scss#new... scss/_form.scss:20: border: 1px solid #CDCDCD; NIT: These greys are not found in the adblockplus.org styleguide (Don't delay publishing as a result, but please do point this out to the product team and ask them to be consistent.) https://codereview.adblockplus.org/29646555/diff/29646566/scss/_form.scss#new... scss/_form.scss:26: .input_round_error On 2017/12/21 16:07:58, saroyanm wrote: > Suggestion: What about calling the classname .invalid or error? > Doesn't look like that input_round_error inherit any style, until it's a > websites module naming convention you are following using underscores. We never use underscores. Perhaps this indicates that this class is added by the platform in some way that is out of your control? https://codereview.adblockplus.org/29646555/diff/29646566/scss/_layout.scss File scss/_layout.scss (right): https://codereview.adblockplus.org/29646555/diff/29646566/scss/_layout.scss#n... scss/_layout.scss:7: background-color: #F3F3F3; Detail: See comment re greys on this page in forms.scss. https://codereview.adblockplus.org/29646555/diff/29646566/scss/_layout.scss#n... scss/_layout.scss:29: .outer-container NIT: This has more to do with vertical alignment than horizontal containment. As a result, perhaps you could name it differently to avoid confusion about what this does? https://codereview.adblockplus.org/29646555/diff/29646566/scss/_layout.scss#n... scss/_layout.scss:45: Where's .phablet-width? https://codereview.adblockplus.org/29646555/diff/29646566/scss/_layout.scss#n... scss/_layout.scss:62: display: inline-table; Not sure why this is necessary? It seems to work similarly without display inline-table and table-cell below? https://codereview.adblockplus.org/29646555/diff/29646566/scss/_layout.scss#n... scss/_layout.scss:64: font-weight: 300; Detail: We decided to go with 400 for adblockplus.org - I suspect we should do the same here too. https://codereview.adblockplus.org/29646555/diff/29646566/scss/_layout.scss#n... scss/_layout.scss:93: strong { font-weight: 700; } NIT/suggest: stronger Work it, make it, do it, makes us...
Thanks Manvel & Julian! I've uploaded a new patch set and addressed your comments. I will also work on implementing these changes to the Backclick preview. https://codereview.adblockplus.org/29646555/diff/29646556/index.html File index.html (right): https://codereview.adblockplus.org/29646555/diff/29646556/index.html#newcode7 index.html:7: <link rel="stylesheet" type="text/css" href="css/main.min.css"> On 2017/12/21 16:07:57, saroyanm wrote: > On 2017/12/21 11:54:05, ire wrote: > > In the actual implementation, I will likely inline these styles because: > > - I'm not sure how to host files with Backclick > Do they have an API documentation in English ? > They site seems to be only in German. No they don't :( I've just been trying to figure out how they do things by trial and also using Google translate for some sentences. > > - The stylesheet isn't that big so we might as well not have an additional > > resource to have to fetch > This is a relatively simple page, we might be able to have a simple Internal CSS > without website-defaults, but I'll leave it up to you and Julian to decide if > having whole websites defaults minified here is necessary or not(it doesn't look > like that to me though). You're right. I will only include the specific files I'm using instead of the whole of website-defaults https://codereview.adblockplus.org/29646555/diff/29646566/index.html File index.html (right): https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode2 index.html:2: <html lang="en" dir="ltr"> 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 https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode4 index.html:4: <meta charset="utf-8"> 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. https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode4 index.html:4: <meta charset="utf-8"> 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. https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode5 index.html:5: <title>Adblock Plus newsletter signup form</title> 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. https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode5 index.html:5: <title>Adblock Plus newsletter signup form</title> 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 > 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. https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode7 index.html:7: <link rel="stylesheet" type="text/css" href="css/main.min.css"> 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) https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode14 index.html:14: <h1 id="title"> 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 ? https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode16 index.html:16: <span>Adblock<strong>Plus</strong></span> 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. https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode16 index.html:16: <span>Adblock<strong>Plus</strong></span> 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. https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode18 index.html:18: <p class="lead"><strong>Sign up for the Adblock Plus newsletter</strong></p> 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. https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode18 index.html:18: <p class="lead"><strong>Sign up for the Adblock Plus newsletter</strong></p> 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. https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode22 index.html:22: <form class="phone-width align-center"> 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. https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode22 index.html:22: <form class="phone-width align-center"> 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? https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode25 index.html:25: <span class="error-message">Please enter a valid email address</span> 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. https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode25 index.html:25: <span class="error-message">Please enter a valid email address</span> 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. https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode30 index.html:30: <button type="submit" class="button secondary">Sign Up</button> 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. https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode32 index.html:32: <p class="text-center"> 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. https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode32 index.html:32: <p class="text-center"> On 2017/12/21 22:06:50, juliandoucette wrote: > NIT: I think that this should be inside a <footer> Done. https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode33 index.html:33: <a href="https://adblockplus.org/en/privacy" target="_blank">Privacy Policy</a> 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 https://codereview.adblockplus.org/29646555/diff/29646566/scss/_content.scss File scss/_content.scss (right): https://codereview.adblockplus.org/29646555/diff/29646566/scss/_content.scss#... scss/_content.scss:5: a On 2017/12/21 22:06:51, juliandoucette wrote: > NIT: should be `.content a` if you are trying to be consistent with > website-defaults? I'm not using the .content class here. This should apply to all links. https://codereview.adblockplus.org/29646555/diff/29646566/scss/_form.scss File scss/_form.scss (right): https://codereview.adblockplus.org/29646555/diff/29646566/scss/_form.scss#new... scss/_form.scss:9: height: 40px; On 2017/12/21 22:06:52, juliandoucette wrote: > NIT: I'd prefer an em value here Done. https://codereview.adblockplus.org/29646555/diff/29646566/scss/_form.scss#new... scss/_form.scss:10: border-radius: 4px; On 2017/12/21 22:06:52, juliandoucette wrote: > NIT: I think rounded edges is inconsistent with http://adblockplus.org > > (Don't delay publishing as a result, but please do point this out to the product > team and ask them to be consistent.) Could you leave a comment in the Spec about this please? https://codereview.adblockplus.org/29646555/diff/29646566/scss/_form.scss#new... scss/_form.scss:20: border: 1px solid #CDCDCD; On 2017/12/21 22:06:52, juliandoucette wrote: > NIT: These greys are not found in the http://adblockplus.org styleguide > > (Don't delay publishing as a result, but please do point this out to the product > team and ask them to be consistent.) This is the same grey from the adblockplusui https://codereview.adblockplus.org/29646555/diff/29646566/scss/_form.scss#new... scss/_form.scss:26: .input_round_error On 2017/12/21 16:07:58, saroyanm wrote: > Suggestion: What about calling the classname .invalid or error? > Doesn't look like that input_round_error inherit any style, until it's a > websites module naming convention you are following using underscores. I was using the class added by the default Backclick template. But I can change that, and will change it to .invalid https://codereview.adblockplus.org/29646555/diff/29646566/scss/_form.scss#new... scss/_form.scss:26: .input_round_error On 2017/12/21 22:06:52, juliandoucette wrote: > On 2017/12/21 16:07:58, saroyanm wrote: > > Suggestion: What about calling the classname .invalid or error? > > Doesn't look like that input_round_error inherit any style, until it's a > > websites module naming convention you are following using underscores. > > We never use underscores. Perhaps this indicates that this class is added by the > platform in some way that is out of your control? See my reply above. https://codereview.adblockplus.org/29646555/diff/29646566/scss/_form.scss#new... scss/_form.scss:47: .button On 2017/12/21 16:07:58, saroyanm wrote: > Why are we introducing new button classes, for the buttons ? > I'll suggest to not introduce them, if there is no reason for that(ex: style not > button element to look like a button) You're right, this was unnecessary. Done. https://codereview.adblockplus.org/29646555/diff/29646566/scss/_vars.scss File scss/_vars.scss (right): https://codereview.adblockplus.org/29646555/diff/29646566/scss/_vars.scss#new... scss/_vars.scss:23: $phone-width: 320px; On 2017/12/21 16:07:59, saroyanm wrote: > Seems like we are only using this variable to set the width of the form(until > I'm missing smth.), I'll suggest to use different variable for the purpose, ex: > form-width. This was for consistency with website-defaults. But it's not relevant anymore because I've removed it.
https://codereview.adblockplus.org/29646555/diff/29646566/index.html File index.html (right): https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode33 index.html:33: <a href="https://adblockplus.org/en/privacy" target="_blank">Privacy Policy</a> 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
Besides of I would encourage to reject the service providers in future that do not provide English documentation, I see that people are pushing this project for the reason I don't know, so I don't intend to stop this, because I assume you had this discussion already and there might be some reason why you agree on current servile provider. Other from that I see all the comments are addressed, so I don't want to stop this, regarding the layout LGTM. https://codereview.adblockplus.org/29646555/diff/29646556/index.html File index.html (right): https://codereview.adblockplus.org/29646555/diff/29646556/index.html#newcode7 index.html:7: <link rel="stylesheet" type="text/css" href="css/main.min.css"> On 2017/12/22 10:33:22, ire wrote: > On 2017/12/21 16:07:57, saroyanm wrote: > > On 2017/12/21 11:54:05, ire wrote: > > > In the actual implementation, I will likely inline these styles because: > > > - I'm not sure how to host files with Backclick > > Do they have an API documentation in English ? > > They site seems to be only in German. > > No they don't :( That's a shame that we have choose a German provider without English support, this shouldn't have happened on the first place. I wonder if that question was raised already ? Anyway -> https://issues.adblockplus.org/ticket/6210#comment:15 > I've just been trying to figure out how they do things by trial and also using > Google translate for some sentences. > > > > - The stylesheet isn't that big so we might as well not have an additional > > > resource to have to fetch > > This is a relatively simple page, we might be able to have a simple Internal > CSS > > without website-defaults, but I'll leave it up to you and Julian to decide if > > having whole websites defaults minified here is necessary or not(it doesn't > look > > like that to me though). > > You're right. I will only include the specific files I'm using instead of the > whole of website-defaults > https://codereview.adblockplus.org/29646555/diff/29646566/index.html File index.html (right): https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode25 index.html:25: <span class="error-message">Please enter a valid email address</span> 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.
Responses. Will be AFK for 30 and then review the latest patchset. https://codereview.adblockplus.org/29646555/diff/29646566/index.html File index.html (right): https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode2 index.html:2: <html lang="en" dir="ltr"> 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. https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode5 index.html:5: <title>Adblock Plus newsletter signup form</title> 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. https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode7 index.html:7: <link rel="stylesheet" type="text/css" href="css/main.min.css"> 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. > (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. https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode14 index.html:14: <h1 id="title"> 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. https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode25 index.html:25: <span class="error-message">Please enter a valid email address</span> 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. https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode30 index.html:30: <button type="submit" class="button secondary">Sign Up</button> 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. https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode33 index.html:33: <a href="https://adblockplus.org/en/privacy" target="_blank">Privacy Policy</a> 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. https://codereview.adblockplus.org/29646555/diff/29646566/scss/_content.scss File scss/_content.scss (right): https://codereview.adblockplus.org/29646555/diff/29646566/scss/_content.scss#... scss/_content.scss:5: a On 2017/12/22 10:33:26, ire wrote: > I'm not using the .content class here. This should apply to all links. You were at the time https://codereview.adblockplus.org/29646555/diff/29646566/index.html#newcode11. Fine with me; anyway. https://codereview.adblockplus.org/29646555/diff/29646566/scss/_form.scss File scss/_form.scss (right): https://codereview.adblockplus.org/29646555/diff/29646566/scss/_form.scss#new... scss/_form.scss:10: border-radius: 4px; On 2017/12/22 10:33:26, ire wrote: > Could you leave a comment in the Spec about this please? Yes. https://codereview.adblockplus.org/29646555/diff/29646566/scss/_form.scss#new... scss/_form.scss:20: border: 1px solid #CDCDCD; On 2017/12/22 10:33:26, ire wrote: > This is the same grey from the adblockplusui My comment stands. I commented in the spec. https://codereview.adblockplus.org/29646555/diff/29646566/scss/_form.scss#new... scss/_form.scss:26: .input_round_error On 2017/12/22 10:33:26, ire wrote: > On 2017/12/21 22:06:52, juliandoucette wrote: > > On 2017/12/21 16:07:58, saroyanm wrote: > > > Suggestion: What about calling the classname .invalid or error? > > > Doesn't look like that input_round_error inherit any style, until it's a > > > websites module naming convention you are following using underscores. > > > > We never use underscores. Perhaps this indicates that this class is added by > the > > platform in some way that is out of your control? > > See my reply above. Acknowledged. https://codereview.adblockplus.org/29646555/diff/29646566/scss/_layout.scss File scss/_layout.scss (right): https://codereview.adblockplus.org/29646555/diff/29646566/scss/_layout.scss#n... scss/_layout.scss:7: background-color: #F3F3F3; On 2017/12/21 22:06:52, juliandoucette wrote: > Detail: See comment re greys on this page in forms.scss. Acknowledged. https://codereview.adblockplus.org/29646555/diff/29646566/scss/_vars.scss File scss/_vars.scss (right): https://codereview.adblockplus.org/29646555/diff/29646566/scss/_vars.scss#new... scss/_vars.scss:23: $phone-width: 320px; On 2017/12/22 10:33:26, ire wrote: > On 2017/12/21 16:07:59, saroyanm wrote: > > Seems like we are only using this variable to set the width of the form(until > > I'm missing smth.), I'll suggest to use different variable for the purpose, > ex: > > form-width. > > This was for consistency with website-defaults. But it's not relevant anymore > because I've removed it. Acknowledged.
Not LGTM Spec comparison and HTML/CSS review. https://codereview.adblockplus.org/29646555/diff/29647600/index.html File index.html (right): https://codereview.adblockplus.org/29646555/diff/29647600/index.html#newcode4 index.html:4: <!-- Website Defaults Standard Metadata --> TOL: Is <!-- website-defaults/includes/meta/standard --> more useful? https://codereview.adblockplus.org/29646555/diff/29647600/index.html#newcode8 index.html:8: <meta charset="utf-8"> Duplicate. https://codereview.adblockplus.org/29646555/diff/29647600/index.html#newcode16 index.html:16: <div class="outer-container"> NIT: This has more to do with vertical alignment than horizontal containment. As a result, perhaps you could name it differently to avoid confusion about what this does? https://codereview.adblockplus.org/29646555/diff/29647600/index.html#newcode20 index.html:20: <figure> TOL: I think this markup makes sense. But I'm not positive. Feedback would be appreciated. https://codereview.adblockplus.org/29646555/diff/29647600/index.html#newcode21 index.html:21: <img src="img/abp-logo.png" srcset="img/abp-logo.svg 2x" alt="ABP inside red octagon"> NIT: The one that you are using looks better than the one in the spec? https://codereview.adblockplus.org/29646555/diff/29647600/index.html#newcode24 index.html:24: <h1 class="lead"><strong>Sign up for the Adblock Plus newsletter</strong></h1> NIT: This heading looks larger than that seen in the spec (I don't mind if you insist on the size in your implementation, just point that out in the spec). https://codereview.adblockplus.org/29646555/diff/29647600/index.html#newcode24 index.html:24: <h1 class="lead"><strong>Sign up for the Adblock Plus newsletter</strong></h1> NIT: I think heading emphasis is enough. We can make this bold via CSS? https://codereview.adblockplus.org/29646555/diff/29647600/index.html#newcode28 index.html:28: <form id="sign-up"> This form doesn't actually work. I can't approve until it works completely. https://codereview.adblockplus.org/29646555/diff/29647600/index.html#newcode29 index.html:29: <label for="email">Email Address *</label> NIT: There seems to be more space below these in the spec. https://codereview.adblockplus.org/29646555/diff/29647600/index.html#newcode31 index.html:31: <span class="error-message">Please enter a valid email address</span> NIT/Suggest: <div> instead of <span> to make it clear that this message appears on a separate line. https://codereview.adblockplus.org/29646555/diff/29647600/scss/_content.scss File scss/_content.scss (right): https://codereview.adblockplus.org/29646555/diff/29647600/scss/_content.scss#... scss/_content.scss:8: color: #077CA6; NIT: This color is darker than the one in the spec? (I like yours better.) https://codereview.adblockplus.org/29646555/diff/29647600/scss/_form.scss File scss/_form.scss (right): https://codereview.adblockplus.org/29646555/diff/29647600/scss/_form.scss#new... scss/_form.scss:13: /* Input field Suggest: Group fields into paragraphs in order to provide consistent spacing. https://codereview.adblockplus.org/29646555/diff/29647600/scss/_form.scss#new... scss/_form.scss:16: input NIT: There seems to be more space below these in the spec
New patch. I still haven't figured out the issue with the form not sending https://codereview.adblockplus.org/29646555/diff/29647600/index.html File index.html (right): https://codereview.adblockplus.org/29646555/diff/29647600/index.html#newcode4 index.html:4: <!-- Website Defaults Standard Metadata --> On 2017/12/22 14:34:30, juliandoucette wrote: > TOL: Is <!-- website-defaults/includes/meta/standard --> more useful? Done. https://codereview.adblockplus.org/29646555/diff/29647600/index.html#newcode8 index.html:8: <meta charset="utf-8"> On 2017/12/22 14:34:30, juliandoucette wrote: > Duplicate. Done. https://codereview.adblockplus.org/29646555/diff/29647600/index.html#newcode16 index.html:16: <div class="outer-container"> On 2017/12/22 14:34:28, juliandoucette wrote: > NIT: This has more to do with vertical alignment than horizontal containment. As > a result, perhaps you could name it differently to avoid confusion about what > this does? I don't think the word ".container" refers horizontal containment only. https://codereview.adblockplus.org/29646555/diff/29647600/index.html#newcode20 index.html:20: <figure> On 2017/12/22 14:34:30, juliandoucette wrote: > TOL: I think this markup makes sense. But I'm not positive. Feedback would be > appreciated. It makes sense to me. I'll look into the figcaption a bit more but later (as we're working on quite a tight deadline here) https://codereview.adblockplus.org/29646555/diff/29647600/index.html#newcode21 index.html:21: <img src="img/abp-logo.png" srcset="img/abp-logo.svg 2x" alt="ABP inside red octagon"> On 2017/12/22 14:34:29, juliandoucette wrote: > NIT: The one that you are using looks better than the one in the spec? I'm actually using the logo from the adblockplusui (if you were looking at the backclick preview) https://codereview.adblockplus.org/29646555/diff/29647600/index.html#newcode24 index.html:24: <h1 class="lead"><strong>Sign up for the Adblock Plus newsletter</strong></h1> On 2017/12/22 14:34:28, juliandoucette wrote: > NIT: I think heading emphasis is enough. We can make this bold via CSS? Done. https://codereview.adblockplus.org/29646555/diff/29647600/index.html#newcode24 index.html:24: <h1 class="lead"><strong>Sign up for the Adblock Plus newsletter</strong></h1> On 2017/12/22 14:34:30, juliandoucette wrote: > NIT: This heading looks larger than that seen in the spec (I don't mind if you > insist on the size in your implementation, just point that out in the spec). I thought it looked better this way but I'll stick to the spec. Done. https://codereview.adblockplus.org/29646555/diff/29647600/index.html#newcode28 index.html:28: <form id="sign-up"> On 2017/12/22 14:34:28, juliandoucette wrote: > This form doesn't actually work. I can't approve until it works completely. As I mentioned in the review description, this was supposed to be about the markup/style, not the functionality as I was still working this out. But we may as well put it together since I've already begun implementation. I'm still working on this issue, will update when i've tested. https://codereview.adblockplus.org/29646555/diff/29647600/index.html#newcode29 index.html:29: <label for="email">Email Address *</label> On 2017/12/22 14:34:28, juliandoucette wrote: > NIT: There seems to be more space below these in the spec. Done. https://codereview.adblockplus.org/29646555/diff/29647600/index.html#newcode31 index.html:31: <span class="error-message">Please enter a valid email address</span> On 2017/12/22 14:34:29, juliandoucette wrote: > NIT/Suggest: <div> instead of <span> to make it clear that this message appears > on a separate line. Done.
Protected this review because it contains links to live preview.
On 2017/12/22 20:57:29, juliandoucette wrote: > Protected this review because it contains links to live preview. Removed links to, and deactivated, said live previews. No need for protection anymore.
Hey Julian, it looks like I've gotten the template to work. I think the reason it wasn't working before was because the email address we were using may have already been subscribed. I'm still not 100% sure but it's working now.
@saroyanm do you want to stay on this review? @ire this is not a full review. Generally, styles do not and probably should match new adblockplus.org and new options page as specified by jeen on Gitlab (despite the screenshots). I suspect that we will have time to update this before it goes live now. https://codereview.adblockplus.org/29646555/diff/29655572/backclick.html File backclick.html (right): https://codereview.adblockplus.org/29646555/diff/29655572/backclick.html#newc... backclick.html:1: <!doctype html> It seems like this file should be generated from separate files containing it's contents and excluded from this repository? https://codereview.adblockplus.org/29646555/diff/29655572/backclick.html#newc... backclick.html:231: <?GIS EMAIL params="id='email' type='email' class='<!--ERROR:ROBINSON-->invalid<!--/ERROR:ROBINSON--><!--ERROR:INCORRECT-EMAIL-->invalid<!--/ERROR:INCORRECT-EMAIL--><!--ERROR:NO-EMAIL-->invalid<!--/ERROR:NO-EMAIL--><!--ALREADY-CONFIRMED--><!--ERROR-->invalid<!--/ERROR--><!--/ALREADY-CONFIRMED--><!--ERROR:ALREADY-ON-10-->invalid<!--/ERROR:ALREADY-ON-10-->' required" mandatory?> NIT/Suggest: Check out how I did this in my minimal DOI template. https://codereview.adblockplus.org/29646555/diff/29655572/backclick.html#newc... backclick.html:233: <!--ERROR:ROBINSON--> I don't think we will use this feature. Please confirm with wspee. https://codereview.adblockplus.org/29646555/diff/29655572/backclick.html#newc... backclick.html:238: <div class="error-message">Please enter a valid email address</div> NIT/TOL: Why no period? https://codereview.adblockplus.org/29646555/diff/29655572/backclick.html#newc... backclick.html:246: <div class="error-message">This email address is already subscribed to the Adblock Plus newsletter</div> Is this necessary? https://codereview.adblockplus.org/29646555/diff/29655572/backclick.html#newc... backclick.html:250: <div class="error-message">This email address is already subscribed to the Adblock Plus newsletter</div> NIT: This seems like more of a success message than an error message? https://codereview.adblockplus.org/29646555/diff/29655572/backclick.html#newc... backclick.html:259: <!--ERROR:NO-PASSWORD--> I don't think these apply (because we don't set a password). https://codereview.adblockplus.org/29646555/diff/29655572/backclick.html#newc... backclick.html:271: <?GIS NEWSLETTER 10 checked params="hidden"?> TOL: What's the difference between putting params="hidden" and hidden="1"? Perhaps params="hidden" will output hidden as a property and not an attribute and have no negative impact on the backend? https://codereview.adblockplus.org/29646555/diff/29655572/backclick.html#newc... backclick.html:273: <input type="hidden" name="REFRESH" value="../web.html"> Is this necessary? https://codereview.adblockplus.org/29646555/diff/29655572/css/main.css File css/main.css (right): https://codereview.adblockplus.org/29646555/diff/29655572/css/main.css#newcode1 css/main.css:1: .unstyled, .unstyled * { margin: 0 !important; padding: 0 !important; border: 0 !important; background: none !important; list-style: none !important; } It seems like this file should be excluded from this repository? https://codereview.adblockplus.org/29646555/diff/29655572/default.html File default.html (right): https://codereview.adblockplus.org/29646555/diff/29655572/default.html#newcode1 default.html:1: <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" I don't think it's necessary to store this in our repository. Perhaps we could *just* put it in the hub wiki or keep an example in Backclick? https://codereview.adblockplus.org/29646555/diff/29655572/gulpfile.js File gulpfile.js (right): https://codereview.adblockplus.org/29646555/diff/29655572/gulpfile.js#newcode2 gulpfile.js:2: * This file is part of adblockplus.org. This is actually part of eyeomail.com? https://codereview.adblockplus.org/29646555/diff/29655572/gulpfile.js#newcode36 gulpfile.js:36: .pipe(sass({outputStyle: "compact"}).on("error", gutil.log)) I don't think we should minify without source maps (which I am assuming that we will not include inline because that would kindof defeat the purpose of minifying in the first place / there is not a lot of css). https://codereview.adblockplus.org/29646555/diff/29655572/gulpfile.js#newcode41 gulpfile.js:41: return fs.readFile('./css/main.css', 'utf8', function (err, styles) { NIT: single quotes. https://codereview.adblockplus.org/29646555/diff/29655572/gulpfile.js#newcode44 gulpfile.js:44: const replacement = `<!-- styles:start --> NIT: Why not shorten this e.g. `<style>${styles}</style>` and place it below? https://codereview.adblockplus.org/29646555/diff/29655572/gulpfile.js#newcode51 gulpfile.js:51: regex: /<!-- styles:start -->([\s\S]*?)<!-- styles:end -->/, NIT: Why not shorten this? e.g. `<!-- styles /-->` https://codereview.adblockplus.org/29646555/diff/29655572/gulpfile.js#newcode53 gulpfile.js:53: paths: ['./index.html', 'backclick.html'], NIT: Can't we *just* do this to backclick.html? https://codereview.adblockplus.org/29646555/diff/29655572/gulpfile.js#newcode54 gulpfile.js:54: recursive: true, NIT: I don't think recursive or silent are necessary for one replacement. https://codereview.adblockplus.org/29646555/diff/29655572/index.html File index.html (right): https://codereview.adblockplus.org/29646555/diff/29655572/index.html#newcode211 index.html:211: <label for="email">Email Address *</label> NIT/Suggest: <a title="Required field">*</a>? https://codereview.adblockplus.org/29646555/diff/29655572/scss/_content.scss File scss/_content.scss (right): https://codereview.adblockplus.org/29646555/diff/29655572/scss/_content.scss#... scss/_content.scss:2: * Content NIT/TOL: I'm not sure that we should put content styles outside of `.content` inside `content.scss`. Perhaps this is 'base.scss` material? https://codereview.adblockplus.org/29646555/diff/29655572/scss/_form.scss File scss/_form.scss (right): https://codereview.adblockplus.org/29646555/diff/29655572/scss/_form.scss#new... scss/_form.scss:2: * Form new abp.org doesn't have form styles yet. I think we can infer these from existing new abp.org and new options page styles. e.g. not-rounded edges and 2px borders on inputs. See https://gitlab.com/eyeo/spec/blob/master/spec/abp/options-page.md
On 2018/01/04 02:34:25, juliandoucette wrote: > @ire this is not a full review. Generally, styles do not and probably should > match new http://adblockplus.org and new options page as specified by jeen on Gitlab > (despite the screenshots). I suspect that we will have time to update this > before it goes live now. Ack. New patch https://codereview.adblockplus.org/29646555/diff/29655572/backclick.html File backclick.html (right): https://codereview.adblockplus.org/29646555/diff/29655572/backclick.html#newc... backclick.html:1: <!doctype html> On 2018/01/04 02:34:16, juliandoucette wrote: > It seems like this file should be generated from separate files containing it's > contents and excluded from this repository? Done. https://codereview.adblockplus.org/29646555/diff/29655572/backclick.html#newc... backclick.html:231: <?GIS EMAIL params="id='email' type='email' class='<!--ERROR:ROBINSON-->invalid<!--/ERROR:ROBINSON--><!--ERROR:INCORRECT-EMAIL-->invalid<!--/ERROR:INCORRECT-EMAIL--><!--ERROR:NO-EMAIL-->invalid<!--/ERROR:NO-EMAIL--><!--ALREADY-CONFIRMED--><!--ERROR-->invalid<!--/ERROR--><!--/ALREADY-CONFIRMED--><!--ERROR:ALREADY-ON-10-->invalid<!--/ERROR:ALREADY-ON-10-->' required" mandatory?> On 2018/01/04 02:34:16, juliandoucette wrote: > NIT/Suggest: Check out how I did this in my minimal DOI template. The difference here being that the classes are applied to the container instead of the input itself? From what I can tell the error messages are also in the HTML whether or not they are being used. I think the default Backclick method is preferable. https://codereview.adblockplus.org/29646555/diff/29655572/backclick.html#newc... backclick.html:233: <!--ERROR:ROBINSON--> On 2018/01/04 02:34:17, juliandoucette wrote: > I don't think we will use this feature. Please confirm with wspee. Asked here: https://issues.adblockplus.org/ticket/6210#comment:19 https://codereview.adblockplus.org/29646555/diff/29655572/backclick.html#newc... backclick.html:238: <div class="error-message">Please enter a valid email address</div> On 2018/01/04 02:34:18, juliandoucette wrote: > NIT/TOL: Why no period? There's no period in the spec and I don't think there should be since it's one sentence and not really a "paragraph text" https://codereview.adblockplus.org/29646555/diff/29655572/backclick.html#newc... backclick.html:246: <div class="error-message">This email address is already subscribed to the Adblock Plus newsletter</div> On 2018/01/04 02:34:17, juliandoucette wrote: > Is this necessary? If the user is already subscribed and they try to subscribe again, they won't know why the form isn't working. We would just get the page refresh and nothing happening https://codereview.adblockplus.org/29646555/diff/29655572/backclick.html#newc... backclick.html:250: <div class="error-message">This email address is already subscribed to the Adblock Plus newsletter</div> On 2018/01/04 02:34:18, juliandoucette wrote: > NIT: This seems like more of a success message than an error message? This message should only appear if the user is trying to subscribe when they are already subscribed, so that seems like an error to me. Also it is an error according to Backclick since it's between the `<!--ALREADY-CONFIRMED--><!--ERROR-->` tag. https://codereview.adblockplus.org/29646555/diff/29655572/backclick.html#newc... backclick.html:259: <!--ERROR:NO-PASSWORD--> On 2018/01/04 02:34:17, juliandoucette wrote: > I don't think these apply (because we don't set a password). I left them mainly for debugging purposes. Since Backclick isn't great with letting us know if there is something wrong, I decided to leave all the potential errors. You're right that they don't really apply, but I'm hesitant to remove them in case we run into some issue with Backclick which would be solved if one of these messages is displayed. https://codereview.adblockplus.org/29646555/diff/29655572/backclick.html#newc... backclick.html:271: <?GIS NEWSLETTER 10 checked params="hidden"?> On 2018/01/04 02:34:18, juliandoucette wrote: > TOL: What's the difference between putting params="hidden" and hidden="1"? > Perhaps params="hidden" will output hidden as a property and not an attribute I don't think the `params` part is necessary, so I will remove it and apply the attribute the same way as `checked`. Adding a value for the hidden attribute, however, is redundant since it's presence/absence is only what is relevant https://codereview.adblockplus.org/29646555/diff/29655572/backclick.html#newc... backclick.html:273: <input type="hidden" name="REFRESH" value="../web.html"> On 2018/01/04 02:34:17, juliandoucette wrote: > Is this necessary? Nope, removed. https://codereview.adblockplus.org/29646555/diff/29655572/css/main.css File css/main.css (right): https://codereview.adblockplus.org/29646555/diff/29655572/css/main.css#newcode1 css/main.css:1: .unstyled, .unstyled * { margin: 0 !important; padding: 0 !important; border: 0 !important; background: none !important; list-style: none !important; } On 2018/01/04 02:34:19, juliandoucette wrote: > It seems like this file should be excluded from this repository? Done. https://codereview.adblockplus.org/29646555/diff/29655572/default.html File default.html (right): https://codereview.adblockplus.org/29646555/diff/29655572/default.html#newcode1 default.html:1: <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" On 2018/01/04 02:34:19, juliandoucette wrote: > I don't think it's necessary to store this in our repository. Perhaps we could > *just* put it in the hub wiki or keep an example in Backclick? The example is already in Backclick, I just kept it here for convenience, but it's no longer needed. Removed https://codereview.adblockplus.org/29646555/diff/29655572/gulpfile.js File gulpfile.js (right): https://codereview.adblockplus.org/29646555/diff/29655572/gulpfile.js#newcode2 gulpfile.js:2: * This file is part of adblockplus.org. On 2018/01/04 02:34:22, juliandoucette wrote: > This is actually part of eyeomail.com? Done. https://codereview.adblockplus.org/29646555/diff/29655572/gulpfile.js#newcode36 gulpfile.js:36: .pipe(sass({outputStyle: "compact"}).on("error", gutil.log)) On 2018/01/04 02:34:19, juliandoucette wrote: > I don't think we should minify without source maps (which I am assuming that we > will not include inline because that would kindof defeat the purpose of > minifying in the first place / there is not a lot of css). This isn't minifying? https://codereview.adblockplus.org/29646555/diff/29655572/gulpfile.js#newcode41 gulpfile.js:41: return fs.readFile('./css/main.css', 'utf8', function (err, styles) { On 2018/01/04 02:34:22, juliandoucette wrote: > NIT: single quotes. Done. https://codereview.adblockplus.org/29646555/diff/29655572/gulpfile.js#newcode44 gulpfile.js:44: const replacement = `<!-- styles:start --> On 2018/01/04 02:34:23, juliandoucette wrote: > NIT: Why not shorten this e.g. `<style>${styles}</style>` and place it below? I think it looks better when the styles start on their own line. Like: ``` <style> // styles here </style> ``` This is consistent with how we nest other HTML elements https://codereview.adblockplus.org/29646555/diff/29655572/gulpfile.js#newcode51 gulpfile.js:51: regex: /<!-- styles:start -->([\s\S]*?)<!-- styles:end -->/, On 2018/01/04 02:34:23, juliandoucette wrote: > NIT: Why not shorten this? e.g. `<!-- styles /-->` This is "sort of" (in mega quotes) a convention for autogenerated inlined content in my experience. Also, I prefer a different format to make it clear that this is injected content. https://codereview.adblockplus.org/29646555/diff/29655572/gulpfile.js#newcode53 gulpfile.js:53: paths: ['./index.html', 'backclick.html'], On 2018/01/04 02:34:22, juliandoucette wrote: > NIT: Can't we *just* do this to backclick.html? I was keeping the other file around so that we can test the HTML/CSS without needing backclick, but we probably don't need that anymore. Removed. https://codereview.adblockplus.org/29646555/diff/29655572/gulpfile.js#newcode54 gulpfile.js:54: recursive: true, On 2018/01/04 02:34:21, juliandoucette wrote: > NIT: I don't think recursive or silent are necessary for one replacement. I just copied the default example here. Is there a reason to *not* use these options? https://codereview.adblockplus.org/29646555/diff/29655572/index.html File index.html (right): https://codereview.adblockplus.org/29646555/diff/29655572/index.html#newcode211 index.html:211: <label for="email">Email Address *</label> On 2018/01/04 02:34:23, juliandoucette wrote: > NIT/Suggest: <a title="Required field">*</a>? I think the <abbr> element is better here? Done. https://codereview.adblockplus.org/29646555/diff/29655572/scss/_content.scss File scss/_content.scss (right): https://codereview.adblockplus.org/29646555/diff/29655572/scss/_content.scss#... scss/_content.scss:2: * Content On 2018/01/04 02:34:24, juliandoucette wrote: > NIT/TOL: I'm not sure that we should put content styles outside of `.content` > inside `content.scss`. Perhaps this is 'base.scss` material? Done. https://codereview.adblockplus.org/29646555/diff/29655572/scss/_form.scss File scss/_form.scss (right): https://codereview.adblockplus.org/29646555/diff/29655572/scss/_form.scss#new... scss/_form.scss:2: * Form On 2018/01/04 02:34:24, juliandoucette wrote: > new http://abp.org doesn't have form styles yet. I think we can infer these from > existing new http://abp.org and new options page styles. e.g. not-rounded edges and 2px > borders on inputs. See > https://gitlab.com/eyeo/spec/blob/master/spec/abp/options-page.md Done.
On 2018/01/04 02:34:25, juliandoucette wrote: > @saroyanm do you want to stay on this review? I'll leave that up to you, in case you think there are things that requires my attention and I can help with something, I'm happy to jump and review. Other from that I completely trust you and Ire on this, thanks for tacking care of this.
https://codereview.adblockplus.org/29646555/diff/29655572/backclick.html File backclick.html (right): https://codereview.adblockplus.org/29646555/diff/29655572/backclick.html#newc... backclick.html:233: <!--ERROR:ROBINSON--> On 2018/01/04 11:14:37, ire wrote: > On 2018/01/04 02:34:17, juliandoucette wrote: > > I don't think we will use this feature. Please confirm with wspee. > > Asked here: https://issues.adblockplus.org/ticket/6210#comment:19 Removed feature.
Updated with specification changes
Added notes. Mostly NITs. https://codereview.adblockplus.org/29646555/diff/29664604/gulpfile.js File gulpfile.js (right): https://codereview.adblockplus.org/29646555/diff/29664604/gulpfile.js#newcode45 gulpfile.js:45: const replacement = `<!-- styles:start --> NIT: (Bringing up an old point, I know) I understand that placing the opening and closing style tags on their own lines makes the end result cleaner. But why replace <! -- styles:(start|end) --> with itself (plus contents within - instead of just replacing the contents within)? And why have an opening **and closing** comment at all if we are replacing everything within? I understand that it may be common, but it doesn't make any sense in this implementation. https://codereview.adblockplus.org/29646555/diff/29664604/gulpfile.js#newcode67 gulpfile.js:67: gulp.task("templates", function() { NIT: This task name is inconsistent with the ones above. If I understand it's function correctly it should be moveTemplates (or similar)? https://codereview.adblockplus.org/29646555/diff/29664604/gulpfile.js#newcode83 gulpfile.js:83: fs.mkdir("./dist", () => { NIT: mkdir implies that a dir doesn't exist already (because the unix command will throw an error in this case). Which looks like a problem because the "template" task runs before the "backclick" task (files are possibly moved to a directory before it is created). I would add a comment somewhere that explains why this doesn't happen or use a mkdir function that doesn't throw an error if a directory already exists. https://codereview.adblockplus.org/29646555/diff/29664604/gulpfile.js#newcode84 gulpfile.js:84: fs.writeFile("./dist/backclick.html", contents.join("")); NIT: A similar issue to the one above applies here. fs.writeFile implies that a file doesn't already exist. Yet there is no "clean" (erase) task in this code. https://codereview.adblockplus.org/29646555/diff/29664604/src/scss/_content.scss File src/scss/_content.scss (right): https://codereview.adblockplus.org/29646555/diff/29664604/src/scss/_content.s... src/scss/_content.scss:16: abbr { text-decoration: none; } NIT: This is a very general rule for a very specific use case. https://codereview.adblockplus.org/29646555/diff/29664604/src/scss/_layout.scss File src/scss/_layout.scss (right): https://codereview.adblockplus.org/29646555/diff/29664604/src/scss/_layout.sc... src/scss/_layout.scss:7: background-color: #F3F3F3; NIT: Colors should be lower case https://codereview.adblockplus.org/29646555/diff/29664604/src/templates/signu... File src/templates/signup.html (right): https://codereview.adblockplus.org/29646555/diff/29664604/src/templates/signu... src/templates/signup.html:26: <figcaption>Adblock <strong>Plus</strong></figcaption> This appears on two lines when I test? https://codereview.adblockplus.org/29646555/diff/29664604/src/templates/signu... src/templates/signup.html:35: <!--UNCONFIRMED--><!--SUCCESS--> NIT: It's inconvenient that these are all visible on the frontend without backclick. It would be nice if they were all disabled by default and we could enable them via a simple interface e.g. the browser console without having to change our code for production e.g. by using the <!-- --> comments to apply classes that change state. https://codereview.adblockplus.org/29646555/diff/29664604/src/templates/signu... src/templates/signup.html:37: <p>You should have received an email with a confirmation link. Please click the link to confirm your email and subscribe</p> NIT: Missing period according to spec https://codereview.adblockplus.org/29646555/diff/29664604/src/templates/signu... src/templates/signup.html:41: <h1>Thank you for subscribing to the <strong>Adblock Plus</strong> newsletter</h1> NIT: Missing period according to spec https://codereview.adblockplus.org/29646555/diff/29664604/src/templates/signu... src/templates/signup.html:76: <!--ERROR:NO-PASSWORD--> If we are going to include these then I think we should at-least make them useful e.g. ask the user to email us if they occur. https://codereview.adblockplus.org/29646555/diff/29664604/src/templates/subsc... File src/templates/subscription-confirmed-email.html (right): https://codereview.adblockplus.org/29646555/diff/29664604/src/templates/subsc... src/templates/subscription-confirmed-email.html:28: <img src="http://asp.backclick.de/email-header/email-header-anmeldung-erfolgreich.png"> See https://gitlab.com/eyeo/spec/merge_requests/118#note_55640724 TL;DR I asked Jeen if we could send the email in essentially plain text first and then develop the template further if we have time. She said yes. --- It may be worth investigating: Can we define *just* a text email and not an HTML one too.
Thanks Julian, responses below and new patch set uploaded https://codereview.adblockplus.org/29646555/diff/29664604/gulpfile.js File gulpfile.js (right): https://codereview.adblockplus.org/29646555/diff/29664604/gulpfile.js#newcode45 gulpfile.js:45: const replacement = `<!-- styles:start --> On 2018/01/22 19:47:52, juliandoucette wrote: > NIT: (Bringing up an old point, I know) > > I understand that placing the opening and closing style tags on their own lines > makes the end result cleaner. But why replace <! -- styles:(start|end) --> with > itself (plus contents within - instead of just replacing the contents within)? The reason I replaced the comments as well was because I found that regex easier to implement. If you can find a regex that would just target the contents of the two comments, let me know. > And why have an opening **and closing** comment at all if we are replacing > everything within? We need both in case we are trying to override styles that already exit. e.g. if we are updating the template and not starting from a blank template that just has the opening and closing comments, nothing in between. If we don't have the consistent opening and closing comments, how would we know where the styles start and stop? We could use the actual `<style>` tags in this case but I think its more future-proof to use the comments in case we for some reason have multiple <style> elements. > I understand that it may be common, but it doesn't make any > sense in this implementation. What I said was common was the format of the styles, i.e. having `styles:start` and `styles:end`. Why doesn't that make any sense? And what do you think would work better? https://codereview.adblockplus.org/29646555/diff/29664604/gulpfile.js#newcode67 gulpfile.js:67: gulp.task("templates", function() { On 2018/01/22 19:47:52, juliandoucette wrote: > NIT: This task name is inconsistent with the ones above. If I understand it's > function correctly it should be moveTemplates (or similar)? Done. https://codereview.adblockplus.org/29646555/diff/29664604/gulpfile.js#newcode83 gulpfile.js:83: fs.mkdir("./dist", () => { On 2018/01/22 19:47:51, juliandoucette wrote: > NIT: mkdir implies that a dir doesn't exist already (because the unix command > will throw an error in this case). Which looks like a problem because the > "template" task runs before the "backclick" task (files are possibly moved to a > directory before it is created). I would add a comment somewhere that explains > why this doesn't happen or use a mkdir function that doesn't throw an error if > a directory already exists. The `mkdir` is actually not needed as the backclick task should only ever be run after the template/css tasks. Removed https://codereview.adblockplus.org/29646555/diff/29664604/gulpfile.js#newcode84 gulpfile.js:84: fs.writeFile("./dist/backclick.html", contents.join("")); On 2018/01/22 19:47:51, juliandoucette wrote: > NIT: A similar issue to the one above applies here. fs.writeFile implies that a > file doesn't already exist. Yet there is no "clean" (erase) task in this code. From my understanding fs.writeFile can be used to replace an existing file as well (https://nodejs.org/api/fs.html#fs_fs_writefile_file_data_options_callback). https://codereview.adblockplus.org/29646555/diff/29664604/src/scss/_content.scss File src/scss/_content.scss (right): https://codereview.adblockplus.org/29646555/diff/29664604/src/scss/_content.s... src/scss/_content.scss:16: abbr { text-decoration: none; } On 2018/01/22 19:47:53, juliandoucette wrote: > NIT: This is a very general rule for a very specific use case. I didn't think it was necessary to introduce a class in this case since we don't have any other use cases for the abbr element. If/when we do, we can add that extra specificity. https://codereview.adblockplus.org/29646555/diff/29664604/src/scss/_layout.scss File src/scss/_layout.scss (right): https://codereview.adblockplus.org/29646555/diff/29664604/src/scss/_layout.sc... src/scss/_layout.scss:7: background-color: #F3F3F3; On 2018/01/22 19:47:54, juliandoucette wrote: > NIT: Colors should be lower case Done. (We should add this to our stylelint) https://codereview.adblockplus.org/29646555/diff/29664604/src/templates/signu... File src/templates/signup.html (right): https://codereview.adblockplus.org/29646555/diff/29664604/src/templates/signu... src/templates/signup.html:26: <figcaption>Adblock <strong>Plus</strong></figcaption> On 2018/01/22 19:47:55, juliandoucette wrote: > This appears on two lines when I test? You mean the "Plus" appears on a second line? I can't seem to reproduce this. Any ideas why it's happening for you? Also, is it also happening on the demo site? https://codereview.adblockplus.org/29646555/diff/29664604/src/templates/signu... src/templates/signup.html:35: <!--UNCONFIRMED--><!--SUCCESS--> On 2018/01/22 19:47:54, juliandoucette wrote: > NIT: It's inconvenient that these are all visible on the frontend without > backclick. It would be nice if they were all disabled by default and we could > enable them via a simple interface e.g. the browser console without having to > change our code for production e.g. by using the <!-- --> comments to apply > classes that change state. I agree that it's inconvenient, but I don't think it's necessarily worth our time to dynamically add/remove these (particularly considering the urgency of this issue). Also, unless we plan to also update the input fields so they look like actual input fields, it may not be really worth it. I update the backclick demo with each new patchset so I think that should work for now. https://codereview.adblockplus.org/29646555/diff/29664604/src/templates/signu... src/templates/signup.html:37: <p>You should have received an email with a confirmation link. Please click the link to confirm your email and subscribe</p> On 2018/01/22 19:47:55, juliandoucette wrote: > NIT: Missing period according to spec Done. https://codereview.adblockplus.org/29646555/diff/29664604/src/templates/signu... src/templates/signup.html:41: <h1>Thank you for subscribing to the <strong>Adblock Plus</strong> newsletter</h1> On 2018/01/22 19:47:55, juliandoucette wrote: > NIT: Missing period according to spec Done. https://codereview.adblockplus.org/29646555/diff/29664604/src/templates/signu... src/templates/signup.html:76: <!--ERROR:NO-PASSWORD--> On 2018/01/22 19:47:56, juliandoucette wrote: > If we are going to include these then I think we should at-least make them > useful e.g. ask the user to email us if they occur. Since they aren't in the spec I'll remove them for now. https://codereview.adblockplus.org/29646555/diff/29664604/src/templates/subsc... File src/templates/subscription-confirmed-email.html (right): https://codereview.adblockplus.org/29646555/diff/29664604/src/templates/subsc... src/templates/subscription-confirmed-email.html:28: <img src="http://asp.backclick.de/email-header/email-header-anmeldung-erfolgreich.png"> On 2018/01/22 19:47:56, juliandoucette wrote: > See https://gitlab.com/eyeo/spec/merge_requests/118#note_55640724 > > TL;DR I asked Jeen if we could send the email in essentially plain text first > and then develop the template further if we have time. She said yes. > > --- > > It may be worth investigating: Can we define *just* a text email and not an HTML > one too. Okay i'll look into it. This review is just for the signup page though, so let's discuss the emails in the review for issue #6215
Thanks Ire! https://codereview.adblockplus.org/29646555/diff/29664604/gulpfile.js File gulpfile.js (right): https://codereview.adblockplus.org/29646555/diff/29664604/gulpfile.js#newcode45 gulpfile.js:45: const replacement = `<!-- styles:start --> On 2018/01/23 08:48:08, ire wrote: > The reason I replaced the comments as well was because I found that regex easier > to implement. If you can find a regex that would just target the contents of the > two comments, let me know. I wasn't able to find a simple regex solution. My understanding of regex is currently very limited. > We need both in case we are trying to override styles that already exit. e.g. if > we are updating the template and not starting from a blank template that just > has the opening and closing comments, nothing in between. If we don't have the > consistent opening and closing comments, how would we know where the styles > start and stop? We could use the actual `<style>` tags in this case but I think > its more future-proof to use the comments in case we for some reason have > multiple <style> elements. But we are not currently, nor are we planning to, override styles that already exist? > What I said was common was the format of the styles, i.e. having `styles:start` > and `styles:end`. Why doesn't that make any sense? And what do you think would > work better? <!-- styles -->, ${styles}, {{ styles }}, or similar. This start and stop format is not necessary unless we are planning to use it to do something more advanced e.g. <style> /* styles that always apply */ <!-- style:start --> /* styles that apply to a pre-complied state and not to a post-compiled state */ <!-- style:end --> </style> Am I missing something? https://codereview.adblockplus.org/29646555/diff/29664604/gulpfile.js#newcode84 gulpfile.js:84: fs.writeFile("./dist/backclick.html", contents.join("")); On 2018/01/23 08:48:08, ire wrote: > On 2018/01/22 19:47:51, juliandoucette wrote: > > NIT: A similar issue to the one above applies here. fs.writeFile implies that > a > > file doesn't already exist. Yet there is no "clean" (erase) task in this code. > > From my understanding fs.writeFile can be used to replace an existing file as > well > (https://nodejs.org/api/fs.html#fs_fs_writefile_file_data_options_callback). Acknowledged. https://codereview.adblockplus.org/29646555/diff/29664604/src/scss/_content.scss File src/scss/_content.scss (right): https://codereview.adblockplus.org/29646555/diff/29664604/src/scss/_content.s... src/scss/_content.scss:16: abbr { text-decoration: none; } On 2018/01/23 08:48:09, ire wrote: > I didn't think it was necessary to introduce a class in this case since we don't > have any other use cases for the abbr element. If/when we do, we can add that > extra specificity. I think we should take the opposite approach. ID: Expectation of a unique use case Class: Expectation of multiple use cases HTML Tag: Expectation of most use cases (exceptions are exceptions) In this case, I think that we are not expecting all abbrs to have no text decoration. I think that we are expecting the one or more abbr of a specific type (required asterisk) to have no text decoration (hence I think class is more appropriate). It is true that it doesn't matter here (hence NIT). But I think we should fix this if we prefer to optimize for maintainability[1]. [1]: If we define maintainability as the ease in which issues can be fixed when they occur. And the potential issue as the addition of an <abbr> tag to this page that should have text decoration. Assuming most use cases for abbr should have text decoration on this page and that none of such cases have occurred yet. https://codereview.adblockplus.org/29646555/diff/29664604/src/templates/signu... File src/templates/signup.html (right): https://codereview.adblockplus.org/29646555/diff/29664604/src/templates/signu... src/templates/signup.html:26: <figcaption>Adblock <strong>Plus</strong></figcaption> On 2018/01/23 08:48:12, ire wrote: > You mean the "Plus" appears on a second line? I can't seem to reproduce this. > Any ideas why it's happening for you? Also, is it also happening on the demo > site? Yes. It seems to be because there is not enough space for the words inside the container in which they are bound. e.g. if you change the figcaption padding from 0 0.5em to 0 0 0 0.5em then there is enough space. --- I discovered this issue by running an http server in the dist directory (were you doing it differently?) and confirmed that it occurs in the demo just-now. I sent you a screenshot. https://codereview.adblockplus.org/29646555/diff/29664604/src/templates/signu... src/templates/signup.html:35: <!--UNCONFIRMED--><!--SUCCESS--> On 2018/01/23 08:48:10, ire wrote: > On 2018/01/22 19:47:54, juliandoucette wrote: > > NIT: It's inconvenient that these are all visible on the frontend without > > backclick. It would be nice if they were all disabled by default and we could > > enable them via a simple interface e.g. the browser console without having to > > change our code for production e.g. by using the <!-- --> comments to apply > > classes that change state. > > I agree that it's inconvenient, but I don't think it's necessarily worth our > time to dynamically add/remove these (particularly considering the urgency of > this issue). Also, unless we plan to also update the input fields so they look > like actual input fields, it may not be really worth it. > > I update the backclick demo with each new patchset so I think that should work > for now. I agree with you in the short-term (hence NIT). And I don't know about the long term. I would do it anyway if it's easy. If not, then we can consider it again after updating this template 2 more times in the future. https://codereview.adblockplus.org/29646555/diff/29664604/src/templates/signu... src/templates/signup.html:76: <!--ERROR:NO-PASSWORD--> On 2018/01/23 08:48:12, ire wrote: > On 2018/01/22 19:47:56, juliandoucette wrote: > > If we are going to include these then I think we should at-least make them > > useful e.g. ask the user to email us if they occur. > > Since they aren't in the spec I'll remove them for now. Acknowledged. https://codereview.adblockplus.org/29646555/diff/29664604/src/templates/subsc... File src/templates/subscription-confirmed-email.html (right): https://codereview.adblockplus.org/29646555/diff/29664604/src/templates/subsc... src/templates/subscription-confirmed-email.html:28: <img src="http://asp.backclick.de/email-header/email-header-anmeldung-erfolgreich.png"> On 2018/01/23 08:48:13, ire wrote: > Okay i'll look into it. This review is just for the signup page though, so let's > discuss the emails in the review for issue #6215 Oh, I didn't know that you saw it that way. Will you please update the review and issue descriptions accordingly (as-needed, e.g. mentioning that this review is about the signup page and not the email templates and contents in this reviews description would be sufficient for this review)? 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
