Skip to content
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

Added subscription form for the news letter #1368

Merged
merged 2 commits into from Aug 20, 2014

Conversation

Projects
None yet
5 participants
@anOddHuman
Copy link

commented Aug 17, 2014

commit1
Closes #1349

@ehashman

This comment has been minimized.

Copy link
Member

commented Aug 17, 2014

I paired with @missincredulous on this, so it would be cool if someone else could provide some PR feedback!

Isn't it pretty/awesome?! :D

@coveralls

This comment has been minimized.

Copy link

commented Aug 17, 2014

Coverage Status

Coverage remained the same when pulling a815592 on missincredulous:master into b39bd28 on openhatch:master.

@brittag

This comment has been minimized.

Copy link
Member

commented Aug 17, 2014

Aw, yay this is nice! A few minor suggestions:

  • The word "newsletter" can link to our archive of newsletters to help people get a sense of what they're subscribing to. :)
  • The "Read more about getting involved »" link should be at the end of the first paragraph (before the "subscribe" text) instead of at the end of the box, since it's directly related to the first paragraph but not as directly related to the subscription invitation.
  • I think there doesn't need to be an extra line of space between "Subscribe to our once-a-month newsletter!" and "Your email address", since the first paragraph doesn't have space between its title and its text.
  • If you change "once-a-month" to "monthly", that might help that sentence fit on one line. :)
@paulproteus

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2014

@brittag , thank you for reviewing this!

@missincredulous , do you think you can look into the above things, over the next 3 days? If so, that's great, and we'll happily wait for you to make this more perfect.

If not, we can just deploy this now, and work on revisions separately.

Let us know what to expect, and we will react accordingly.

Many thanks for your first pull request, and I hope we see you around!

@anOddHuman

This comment has been minimized.

Copy link
Author

commented Aug 17, 2014

I'll be happy to try and make those edits over the next 72 hours!

On Sun, Aug 17, 2014 at 7:03 PM, Asheesh Laroia notifications@github.com
wrote:

@brittag https://github.com/brittag , thank you for reviewing this?

@missincredulous https://github.com/missincredulous , do you think you
can look into the above things, over the next 3 days? If so, that's great,
and we'll happily wait for you to make this more perfect.

If not, we can just deploy this now, and work on revisions separately.

Let us know what to expect, and we will react accordingly.

Many thanks for your first pull request, and I hope we see you around!


Reply to this email directly or view it on GitHub
#1368 (comment)
.

@anOddHuman

This comment has been minimized.

Copy link
Author

commented Aug 19, 2014

Added changes as requested:

  1. Moved read more link to it's proper paragraph.
  2. Added the same spacing for both strong elements.
  3. Changed "Once a month" to "monthly".

commit2

@coveralls

This comment has been minimized.

Copy link

commented Aug 19, 2014

Coverage Status

Coverage decreased (-0.01%) when pulling 19bd80e on missincredulous:master into b39bd28 on openhatch:master.

@brittag

This comment has been minimized.

Copy link
Member

commented Aug 19, 2014

Thanks @missincredulous, this looks good! Interestingly, when I put in my email address to see if it works, I don't seem to get an emailed confirmation message in my inbox, but I also don't seem to get an emailed confirmation message if I put my email address into the box in the blog sidebar. Maybe @paulproteus can check to make sure this is working on a functionality level?

@paulproteus

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2014

Oh dear!

I'll check re: functionality.

@paulproteus

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2014

...well, the mailman queue was backed up on the main OpenHatch server.

I have some monitoring for this, but what I should set up is self-healing.

)-:

Regardless, @brittag , try now. I think it'll work great.

@missincredulous , I think we still need a licensing statement from you -- see http://openhatch.readthedocs.org/en/latest/getting_started/handling_contributions.html#permit-us-to-share-your-work for more on that.

@brittag

This comment has been minimized.

Copy link
Member

commented Aug 20, 2014

Yes, works now!

@anOddHuman

This comment has been minimized.

Copy link
Author

commented Aug 20, 2014

I actually sent an e-mail in on Sunday regarding the license statement. I'm
guessing it is not there?

Kristina.

On Wed, Aug 20, 2014 at 2:21 AM, Asheesh Laroia notifications@github.com
wrote:

...well, the mailman queue was backed up on the main OpenHatch server.

I have some monitoring for this, but what I should set up is self-healing.

)-:

Regardless, @brittag https://github.com/brittag , try now. I think
it'll work great.

@missincredulous https://github.com/missincredulous , I think we still
need a licensing statement from you -- see
http://openhatch.readthedocs.org/en/latest/getting_started/handling_contributions.html#permit-us-to-share-your-work
for more on that.


Reply to this email directly or view it on GitHub
#1368 (comment)
.

@ehashman

This comment has been minimized.

Copy link
Member

commented Aug 20, 2014

@paulproteus: It was backed up in the moderator queue. I just approved it.

I'm going to merge this, now that we have a @brittag and @paulproteus +1! :D

ehashman added a commit that referenced this pull request Aug 20, 2014

Merge pull request #1368 from missincredulous/master
Added subscription form for the news letter

@ehashman ehashman merged commit e4c253e into openhatch:master Aug 20, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.