Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(414)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 months ago by ire
Modified:
8 months, 3 weeks ago
Visibility:
Public.

Description

Issue : 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+565 lines, -0 lines) Patch
A .hgignore View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -0 lines 0 comments Download
A README.md View 1 2 3 4 5 6 7 8 9 10 1 chunk +3 lines, -0 lines 0 comments Download
A gulpfile.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +102 lines, -0 lines 0 comments Download
A package.json View 1 2 3 4 5 6 7 8 9 10 1 chunk +22 lines, -0 lines 0 comments Download
A src/scss/_content.scss View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +19 lines, -0 lines 0 comments Download
A src/scss/_form.scss View 1 2 3 4 5 6 7 8 9 10 1 chunk +56 lines, -0 lines 0 comments Download
A src/scss/_layout.scss View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +107 lines, -0 lines 0 comments Download
A src/scss/_vars.scss View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +23 lines, -0 lines 0 comments Download
A src/scss/main.scss View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +13 lines, -0 lines 0 comments Download
A src/templates/confirm-email.html View 1 2 3 4 5 6 7 8 9 10 1 chunk +76 lines, -0 lines 0 comments Download
A src/templates/signup.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +88 lines, -0 lines 0 comments Download
A src/templates/subscription-confirmed-email.html View 1 2 3 4 5 6 7 8 9 10 1 chunk +54 lines, -0 lines 0 comments Download

Messages

Total messages: 22
ire
10 months ago (2017-12-21 11:25:54 UTC) #1
ire
@Manvel please take a look at this when you can. I've added some background information ...
10 months ago (2017-12-21 11:51:24 UTC) #2
ire
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 ...
10 months ago (2017-12-21 11:54:06 UTC) #3
saroyanm
Comments are ready. Some of my comments may or may not be relevant, because as ...
10 months ago (2017-12-21 16:07:59 UTC) #4
juliandoucette
CC wspee @ire Here are my first impressions. @wspee saroyanm already added his first impressions. ...
10 months ago (2017-12-21 22:06:53 UTC) #5
ire
Thanks Manvel & Julian! I've uploaded a new patch set and addressed your comments. I ...
10 months ago (2017-12-22 10:33:27 UTC) #6
ire
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: ...
10 months ago (2017-12-22 11:46:28 UTC) #7
saroyanm
Besides of I would encourage to reject the service providers in future that do not ...
10 months ago (2017-12-22 12:26:11 UTC) #8
juliandoucette
Responses. Will be AFK for 30 and then review the latest patchset. https://codereview.adblockplus.org/29646555/diff/29646566/index.html File index.html ...
10 months ago (2017-12-22 13:32:24 UTC) #9
juliandoucette
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 ...
10 months ago (2017-12-22 14:34:32 UTC) #10
ire
New patch. I still haven't figured out the issue with the form not sending https://codereview.adblockplus.org/29646555/diff/29647600/index.html ...
10 months ago (2017-12-22 16:29:38 UTC) #11
juliandoucette
Protected this review because it contains links to live preview.
10 months ago (2017-12-22 20:57:29 UTC) #12
juliandoucette
On 2017/12/22 20:57:29, juliandoucette wrote: > Protected this review because it contains links to live ...
10 months ago (2017-12-22 21:07:50 UTC) #13
ire
Hey Julian, it looks like I've gotten the template to work. I think the reason ...
9 months, 2 weeks ago (2018-01-03 11:18:54 UTC) #14
juliandoucette
@saroyanm do you want to stay on this review? @ire this is not a full ...
9 months, 2 weeks ago (2018-01-04 02:34:25 UTC) #15
ire
On 2018/01/04 02:34:25, juliandoucette wrote: > @ire this is not a full review. Generally, styles ...
9 months, 2 weeks ago (2018-01-04 11:14:44 UTC) #16
saroyanm
On 2018/01/04 02:34:25, juliandoucette wrote: > @saroyanm do you want to stay on this review? ...
9 months, 2 weeks ago (2018-01-04 12:12:30 UTC) #17
ire
https://codereview.adblockplus.org/29646555/diff/29655572/backclick.html File backclick.html (right): https://codereview.adblockplus.org/29646555/diff/29655572/backclick.html#newcode233 backclick.html:233: <!--ERROR:ROBINSON--> On 2018/01/04 11:14:37, ire wrote: > On 2018/01/04 ...
9 months, 2 weeks ago (2018-01-08 11:02:38 UTC) #18
ire
Updated with specification changes
9 months, 1 week ago (2018-01-12 13:20:09 UTC) #19
juliandoucette
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 ...
9 months ago (2018-01-22 19:47:58 UTC) #20
ire
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: ...
8 months, 4 weeks ago (2018-01-23 08:48:14 UTC) #21
juliandoucette
8 months, 4 weeks ago (2018-01-23 17:00:49 UTC) #22
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)?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5