Added in UK sort code and Duration yy_mm

[jh3y]

Added in formatting and validation for UK sort code and duration in years and months. All tests pass and demo code in place as well as wiki pages.

[omarshammas]

Thanks for contributing @jheytompkins I appreciate your help. Just a few changes, and I'll be happy to merge it in.

• I added a few more tests https://gist.github.com/omarshammas/6796428 to the uk sort code and it fails. Try adding these tests and see the errors. Ignore the restrictNumeric and restrictAlphaNumeric errors it is a known issue and is documented here.
• I think the validator is the source of errors. You check to see if the string length is less than 12. instead it should check if it contains 6 digits. People may only use the validator and not jointly with the formatter so there could be cases where the string is 123456 which is valid. However, with the current setup it will fail. Something like /^\d{2}[\s|\-]*\d{2}[\s|\-]*\d{2}\s*\$\ would be better. This way it works for a whole range of inputs 123456, 12 34 56, or even stranger ones 12 -34- 56.
• Backspacing causes some strange behavior. Backspacing on '12 - 34 - ' results in '12 - 34 -' when it should be '12 - 3'.
• If you could rename the yy_mm fields to Date instead of Time to be more consistent with the other dd_mm_yyyy and yyyy_mm_dd fields. Time is usually associate with hours, minutes and seconds such as hh:mm:ss.
• In the yy_mm tests if you add the following tests to the validator to ensure they pass. 1- invalid months such as 13 / 13. 2- strings that contain non digits such as 13 / 0i
• Lastly the fields that you added to the formancejs.html, they're being displayed in alphabetical order so if you could modify changes to maintain that order.

If you have any questions or concerns just put them in the comments and we can discuss them. Thanks again!

[jh3y]

Hi.

Thanks for the feedback. No problem I'll get that in this evening.

Sent from my iPhone

On 2 Oct 2013, at 17:56, Omar Shammas notifications@github.com wrote:

Thanks for contributing @jheytompkins I appreciate your help. Just a few changes, and I'll be happy to merge it in.

I added a few more tests https://gist.github.com/omarshammas/6796428 to the uk sort code and it fails. Try adding these tests and see the errors. Ignore the restrictNumeric and restrictAlphaNumeric errors it is a known issue and is documented here.

I think the validator is the source of errors. You check to see if the string length is less than 12. instead it should check if it contains 6 digits. People may only use the validator and not jointly with the formatter so there could be cases where the string is 123456 which is valid. However, with the current setup it will fail. Something like /^\d{2}[\s|-]\d{2}[\s|-]\d{2}\s*\$\ would be better. This way it works for a whole range of inputs 123456, 12 34 56, or even stranger ones 12 -34- 56.

Backspacing causes some strange behavior. Backspacing on '12 - 34 - ' results in '12 - 34 -' when it should be '12 - 3'.

If you could rename the yy_mm fields to Date instead of Time to be more consistent with the other dd_mm_yyyy and yyyy_mm_dd fields. Time is usually associate with hours, minutes and seconds such as hh:mm:ss.

In the time yy_mm tests if you add the following tests to the validator to ensure they pass. 1- invalid months such as 13 13 / 13. 2- strings that contain non digits such as 13 / 0i

Lastly the fields that you added to the formancejs.html, they're being displayed in alphabetical order so if you could modify changes to maintain that order.

If you have any questions or concerns just put them in the comments and we can discuss them. Thanks again!

— Reply to this email directly or view it on GitHub.

[jh3y]

Will try and get the relevant changes in for yy_mm in later.

[jh3y]

The changes for yy_mm have been made now including extra tests that pass such. Also the demo ordering is maintained now as I have placed the two fields in the correct places.

[omarshammas]

Here is some more feedback and we should be good to go.

yy / mm

• val_mm_yy is included but there are no tests. This method returns a basic js object (hash) with month and year keys, but other date fields (yyyy_mm_dd and dd_mm_yyyy) return a js Date object. It is important to maintain consistency so it should return a date object as well. This is a special case because it is not clear what the expected millennia is when dealing with the text 20 / 10. Does it refer to October 2020 or October 1920? I think a reasonable cutoff would be around the 50, because if you say 98 you're probably referring to 1998 and if you say 00 you're probably referring to 2013. What are your thoughts? Below is a recap of what I think
  • It should return a Date object to be consistent with the other date fields.
  • If the year is 0 - 49 it will use the 2000, and if 50 - 99 it should use 1900.
  • An enhancement would be to allow overriding this behavior with a data attribute. <input data-formance_millenia='2000' /> would use only the 2000 millennia so 98 / 10 would refer to October 2098. However, this is an enhancement and can be added at a later point.
• 00 / 00 is not a valid date but it passes. The year 00 exists but month 00 does not.
• To be consistent with the other date fields 13 / 1 should pass. It is debatable whether this should validate and that can be opened as a separate issue, but in the mean time it is important to be consistent across all fields and it should pass.
• add these tests to the formatter, they already pass. But useful in the future if the internal implementation changes we know we didn't break anything.

uk_sort_code looks good. If it was in a separate pull request I would merge it in, but I will hold off until the above changes are made. I will update the How to Contribute docs to make sure fields are submitted independent of one another to avoid such hold ups in the future.

I'm still figuring out the how to manage this community, so thanks again for your patience and contributions!

[jh3y]

Hey,

Valid points about 'yy_mm'. However, the intended use of yy_mm isn't to return a date but a duration hence why I originally called it time_yy_mm.

In many cases in forms we see questions asking for the user to input some kind of duration, examples such as "how long have you held this account?", "how long did you hold this position?" etc.

So with that in mind, '20 / 10' would mean twenty years and ten months and not October 2020.

So '00 / 00' would in fact mean 0 years and 0 months.

Correct, 13 / 1 should be valid as 13 years and 1 months. I'll take a look at that and I'll take a look at those tests for the formatter.

No trouble, anything I can help with, let me know and I'll be happy to help.

@jheytompkins

[omarshammas]

Ahhhh, now I understand. Initially, I thought it was mean to represent a date such as a credit expiration but now I see how it can be used to represent time duration.

Alright, so I think it is best to rename the field it to something other than yy_mm because that will be confusing especially when it passes with input of 1 / 10 . I like time_yy_mm or duration_yy_mm, what do you think?

As well it should handle valid input such as 0 / 24 which is 0 years and 24 months.

One last thing there still appears to a problem with the uk sort code formatter when backspacing. I will take a closer look tomorrow to see why it is happening.

[jh3y]

Hey.

Yeah I willl change the name to time I think. I prefer the sound of that haha. I think 1 / 10 should pass you're right but I wasn't too sure about inputs like 0 / 24. I will make sure it passes though and we could always come back to it if it's a need.

That formatter bug is strange, which browser is this in?

Because I didn't get it the other day but I have had it happen. This is the same for me with the date formatters such as dd_mm_yyyy. Sometimes I can press say 0 then 2 and I get 02 / 02 / input.

I think credit/debit card expiration would be very good to include so I will look to get that put in there. I was also going to look at age limit validation too as this has come up a couple of times. I was thinking of possible ways we may be able to somehow pass the age in as a configurable. If not, just stick to say 18, 21, 25.

@jheytompkins

[jh3y]

Added in those tests and renamed. Updated demo accordingly.

@jheytompkins

[omarshammas]

I merged in both fields, and fixed the backspace problem. It was listening to the keypress event instead of the keydown event.

I've learned quite a bite from this experience, and will be making changes to the project. First revamping the tests to be clearer and more concise. Second, I'm changing the structure of the fields because there is a lot of repetition and frankly it is quite messy and intimidating for potential contributors. I took the stripe payment.js as a base, but clearly it wasn't designed to be extended.

So stay tuned, and thanks again for your contributions.

[jh3y]

Is there anything I can help with? I'm quite happy to get involved.

I am looking to add more fields and generally help out if you need it.

@jheytompkins

[omarshammas]

Great, I appreciate it.

The improvements I mentioned are almost complete. Briefly, there is a FormanceField class which all other fields will inherit from. This handles a lot of the boiler plate code necessary when adding a new field, which I'm sure you noticed was quite taxing.

Now when adding a new field, we just need to create a subclass and implement a few functions such as restrict, format, format_backspace, format_paste, and validate.

Similarly, the tests have been updated to be clearer and more concise and eliminate a lot of the boiler plate code needed to set up each scenario.

The changes are on the revamp_fields branch, which I will be merging into master over the weekend. This makes the code base cleaner, easier to maintain, and lowers the barrier for new contributors.

I will merge these changes in over this thanksgiving weekend. Once merged, I would appreciate your feedback for converting both time_yy_mm and uk_sort_code fields to the new 'structure'.

[jh3y]

Hey.

Sorry took like an age to respond to this.

So you're basically refactoring a lot to try and make it a little easier to add in new fields I take it.

Yeah I can provide feedback of course. I'll be looking to add in new fields as well soon.

@jheytompkins

[omarshammas]

It's all good, life gets busy.

The changes are almost complete and will be merged into master, the only big thing left is to update the uk_sort_code and time_yy_mm fields. This all lives on the revamp branch https://github.com/omarshammas/jquery.formance/tree/revamp

So when you have some time, I would appreciate your feedback as you update those two fields. If you are busy no problem, I can update them.

Lastly, I updated the contributing page with a brief walkthrough on contributing with the new structure, which may be helpful. https://github.com/omarshammas/jquery.formance/wiki/Contributing

[jh3y]

Hey!

I am really sorry I've been quite busy with some other things. I am going to try and get some time to take a look at this as I am keen to see the new structure.

Sorry again

@jheytompkins

[jh3y]

Checked out the new contributing page. Very good!

@jheytompkins