-
Notifications
You must be signed in to change notification settings - Fork 145
Add email subscription form to / and /newsletters #19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
newsletters.md
Outdated
| {%- endfor -%} | ||
| </ul> | ||
|
|
||
| {% include newsletter-signup.html %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest moving this to the top of the page, probably right after the end of the YAML header (line 11) so it displays below the Newsletters page heading but above the list. The reason is that the list of newsletters is going to get longer over time, so this box will move further down the page few people ever see it.
You may also wish to add it to each newsletter (in the same position right below the end of the YAML header), as those pages may receive a lot of incoming link traffic. There are a few ways to do this, such as creating a layout special to newsletters, putting it in the page layout triggered by a news_signup: true option in the YAML header, or just putting the {% include %} in each newsletter---the later is the easiest for now, so I'd suggest that.
Otherwise this LGTM. I tested and it took me to the mailchimp page and from there back to the live site, but they didn't ask me for double opt-in, so maybe it's in some sort of test mode?
|
requires rebase on rebased #18. |
|
rebase plz 🙏 |
|
Rebased and addressed @harding's good feedback. The form is now above the list of newsletters, and I've added a |
|
Tested ACK 41dead2, a rather unfortunate commit id, with the caveat that a test of signup doesn't do double opt-in, so I think the list is in test mode or Mailchimp doesn't follow best practices. |
I think Satoshi is trying to communicate with us. |
|
@jamesob is out of office today, so I've gone ahead and pushed a new branch. Changes:
|
|
Fully tested ACK 258bdbf Thanks! |
Newsletters: add #19 (2018-10-30)
Note that this is based on #18 which should be tested and merged first.