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

Adding newsletter time setting #712

Merged
merged 1 commit into from Jan 22, 2015
Merged

Conversation

anthonymayer
Copy link
Contributor

Adding newsletterTime setting.

Still todo

  • Add tests
  • Add some more translations

@SachaG
Copy link
Contributor

SachaG commented Jan 20, 2015

Should I merge it now, or wait until you add some tests?

@anthonymayer
Copy link
Contributor Author

If you think it's good you're more than welcome to merge it and I can add tests as a follow up. Part of the reason I wanted to add tests is that I don't actually use the newsletter so I've only tested the setting part manually, I don't know the best way to test that the newsletter actually gets sent.

@SachaG
Copy link
Contributor

SachaG commented Jan 21, 2015

Does the time use 24-hour or AM/PM notation? Also, can we make the default time 09:00 AM?

@anthonymayer
Copy link
Contributor Author

@SachaG: Changing the default to 9:00 AM isn't a problem. I used 00:00 because that's what it looked like the default is currently. The time notation is actually based on the user's browser settings, because it uses the HTML5 time input tag. It looks like it always returns 24 hr values though no matter what the user sees.

@SachaG
Copy link
Contributor

SachaG commented Jan 22, 2015

OK, sounds good :)

SachaG added a commit that referenced this pull request Jan 22, 2015
@SachaG SachaG merged commit d45958f into VulcanJS:devel Jan 22, 2015
@anthonymayer anthonymayer deleted the newsletter-time branch February 1, 2015 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants