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 new parameter #85

Merged
merged 2 commits into from Nov 26, 2018
Merged

Adding new parameter #85

merged 2 commits into from Nov 26, 2018

Conversation

Psychosynthesis
Copy link
Contributor

Hello.

Sometimes it is very convenient to be able to turn off the display of the timer on the page.

@walmik
Copy link
Owner

walmik commented Oct 17, 2018

Hi @Psychosynthesis ! Thank you for your interest in improving the timer plugin. Please do note that PRs need to be created with adequate tests added for the features they introduce/update. Additionally could you please explain a real world scenario where starting timer in a hidden way would be useful? I just want to make sure we dont introduce something that could ve been entirely handled via setInterval or setTimeout alone.

@Psychosynthesis
Copy link
Contributor Author

Hi!
The example is rather trivial: if the application specifies the default date in a human-readable format (such as "1h29m"), and already uses your library somewhere, then, if necessary, you can simply use your timer with the parameter "hidden" instead of setInterval \ setTimeout and, at least, get rid of the extra conversion in milliseconds.

@Psychosynthesis
Copy link
Contributor Author

Well, in general, I could come up with a couple more scenarios, of course ...

... but I will say shortly. In my particular case, it seemed to me that using your library was simply more convenient than setInterval \ setTimeout, but I did not need the output to the block.

As a result, I had to add style to hide the block with the timer.

@walmik
Copy link
Owner

walmik commented Oct 18, 2018

It sure makes sense. I think it’s a good thing to add. Please add few tests to your PR to cover your feature.

@Psychosynthesis
Copy link
Contributor Author

Sorry, I believe that I do not fully understand how testing works in your project. Also, I see that you are using grunt for this.

I guess I can not finish the job due to lack of experience.

@walmik
Copy link
Owner

walmik commented Oct 18, 2018

Dont worry, I understand. I ll add the tests later. For now please bump the package minor version as part of this PR. Basically make it 0.9.0 in this file on this line:
https://github.com/walmik/timer.jquery/blob/master/package.json#L5

@walmik
Copy link
Owner

walmik commented Nov 26, 2018

@Psychosynthesis i m merging this in. I ll update the package and add tests.

@walmik walmik merged commit 50ffc31 into walmik:master Nov 26, 2018
@Psychosynthesis
Copy link
Contributor Author

Nice, thank you

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