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

Add Hours/bindEnglishTo and fix for words being zero #7

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

WadeTheFade
Copy link

resolves #3 and adds #5 and #6

I got it working, but was unable to resolve the testing.

Merging to develop in case someone can address before I get a chance.

@WadeTheFade
Copy link
Author

I've found an issue with bindEnglishTo when selecting different scheduleTypes.

There are a few different approaches to fixing.. my first impression is to only call toEnglishString() is validate passes.

I'll try to address this and the failing checks in the next commit.

@kingofzeal
Copy link
Contributor

This looks good.

Would it be possible to get some unit tests wired up to test the new hourly options as well? I would want to make sure that especially the cron output is what we would expect for a given combinations of options (and vice versa, a provided cron string updates the DOM with the correct options).

@WadeTheFade
Copy link
Author

WadeTheFade commented Aug 13, 2018

Yeah there is a bug in the monthly/yearly selection when bindEnglishTo() when loading a default schedule that isn't monthly/yearly:
Not implemented: Monthly.undefined.toEnglishString.

I'll try to address this and the unit tests around hourly soon

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.

3 participants