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 minlength option for leading zeros #11

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

Conversation

aaronholla
Copy link

This Issue was closed but i recently felt the need for exactly this functionality. The response to this issue was that it wouldn't be possible because of the max value would stop the timer if the value hit the max. But it is possible if you think of it from a minimum instead of maximum.

Heres my solution I added an optional minlength value that would default to 1. Then on each render added a check to see if the number of digits is below the min if so it will add leading zeros. If the value is larger then the minlength it will still continue on ticking. It would only add leading zeros if it is needed.

This is useful when your making a clock or a minutes ticker in a timer. For example 8:04 if you had a ticker on the minutes it would be 8:4. It is like this because of the Number() function used to get the value, it strips away any leading zeros.

To give a few examples with a minlength of 2 :

  • Value of 4, it will add a leading zero to make it 04.
  • Value of 15, will not add zeros will stay 15
  • Value of 350, also will not add zeros stays 350

if digits.length < @options.minlength
while (digits.length < @options.minlength)
digits = "0" + digits
digits.split( '' )
Copy link
Member

Choose a reason for hiding this comment

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

I think you meant to keep this outside the while and actually make use of the split string here, i.e.

digits = digits.split('')

Considering the following code only uses digits.length and digits[] you don’t even need to split it tbh.

Copy link
Author

Choose a reason for hiding this comment

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

Oh wow didn't notice that I hadn't even set digits equal to the split.

@jkintscher
Copy link
Member

Thanks for making this! Just left a minor comment on the diff but once that’s fixed this would be a nice addition to the plugin.

@aaronholla
Copy link
Author

Alright I removed the split from the render function in both files.

@@ -64,6 +65,7 @@ class Tick
delay : options.delay or 1000
separators: if options.separators? then options.separators else false
autostart : if options.autostart? then options.autostart else true
minlength : options.minlength or 1
Copy link
Member

Choose a reason for hiding this comment

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

Since this is being treated as a Number in the following implementation you should make sure minlength will always be set to an integer. Your check here will set it to any truthy value.

Consider changing this to something like minlength: parseInt(options.minlength, 10) or 1

@aaronholla
Copy link
Author

Alright I added ParseInt to minlength. Although I noticed a bug that I can't seem to figure out.

When setting minlength to a large number and using separators the separators get placed in the wrong positions. Im not sure I fully understand how the separators get placed into their proper positions.

@aaronholla
Copy link
Author

Okay I think I figured out a solution for the separators. For every "0" put in place I put a new empty element (representing a digit) on the front of the separators array.

@jkintscher
Copy link
Member

Thanks for updating the parseInt stuff. The other problem you’re seeing is with JavaScript integers. When you use numbers that exceed the valid scope of the Number type the default incrementing logic does not work.

You can somehow try to make that work by using a custom callback for the incremental but you should revert aaronholla@54c7fca as it actually does not fix the underlying problem.

The rest looks good to me though, so +1 I’ll happily merge it after that!

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.

2 participants