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 support for days #68

Merged
merged 1 commit into from Jun 9, 2016
Merged

Add support for days #68

merged 1 commit into from Jun 9, 2016

Conversation

kerro
Copy link
Contributor

@kerro kerro commented Jun 8, 2016

Feature #15

@walmik
Copy link
Owner

walmik commented Jun 8, 2016

@kerro Firstly thank you so much for taking the time to build this out! It's a very important feature and has been a while since it s pending and no one (including me) had taken a stab at it.

Secondly I think you ve directly edited the transpiled timer.jquery.js file from the dist directory. Ideally the changes should be done within the files in the src folder. It involves writing the code in ES6 and using webpack with babel to transpile it. The npm run tasks required to do this are all available for in this repo itself hence it should be straight forward. Please let me know if you are comfortable with doing this PR in ES6. (I believe you will be able to) If not, I can help (Soon I ll add a How to contribute file in this repo). It s really easy for me to take these changes and do it all myself, but I really want you to be associated with this change in the git history :)

Lemme know your thoughts and thanks again for taking up this long-awaited feature for development!

@kerro
Copy link
Contributor Author

kerro commented Jun 8, 2016

I already did it like you said using webpack.
The changes are done in src.

@walmik
Copy link
Owner

walmik commented Jun 8, 2016

Terribly silly of me :bowtie:

I ll go through these changes and play with it a bit and get back later today with comments.

@@ -262,6 +262,7 @@
PLUGIN_NAME: 'timer',
TIMER_RUNNING: 'running',
TIMER_PAUSED: 'paused',
DAYINSECOND: 86400,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename this to DAYINSECONDS

@walmik
Copy link
Owner

walmik commented Jun 9, 2016

I ve reviewed this PR and suggested some minor improvements. Aside from that this looks great. Once you update this PR with the suggested changes, I ll merge it right in. May I also ask you to please bump the version to 0.6.0 as this is a new feature addition?

@kerro
Copy link
Contributor Author

kerro commented Jun 9, 2016

I updated files in dist directory ;-)

@walmik
Copy link
Owner

walmik commented Jun 9, 2016

Looks great 👍 thank you very much @kerro !

@walmik walmik merged commit ae1d6aa into walmik:master Jun 9, 2016
@kerro kerro deleted the feature-15 branch June 9, 2016 20:07
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