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 freeform number entry to SC.DateFieldView #796

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@nicolasbadia
Copy link
Member

nicolasbadia commented Jun 11, 2012

Here is my work to solve the issue #783.
This patch keep the actual way that SC.DateFieldView work and add
support for freeform number entry.
You still have to navigate between sub-fields but you can now press
numbers to change the value of the date or time. If the pressed number
create an impossible date, (example: 25 hours), the value will not be
updated.
Add support for: %Y, %y, %m, %d, %H, %I, %M, %S

Add support for freeform number entry to SC.DateFieldView
Here is my work to solve the issue #783.
This patch keep the actual way that SC.DateFieldView work and add
support for freeform number entry.
You still have to navigate between sub-fields but you can now press
numbers to change the value of the date or time. If the pressed number
create an impossible date, (example: 25 hours), the value will not be
updated.
Add support for: %Y, %y, %m, %d, %H, %I, %M, %S
@dcporter

This comment has been minimized.

Copy link
Member

dcporter commented Jun 13, 2012

This is a great idea and looks (at a real quick glance) to be well-implemented. (Since keypress detection is an area where things are getting weird ((i.e. the latest Safari has changed its event model for the better-except-for-compatibility)), could you be sure to test this in the latest Safari as well as Chrome?)

And of course: UNIT TESTS. Before this can get merged in, this sort of nuanced feature absolutely needs to have some unit tests, to prevent it from getting inadvertently broken in the future.

@publickeating

This comment has been minimized.

Copy link
Member

publickeating commented Jun 14, 2012

Awesome work taking this new feature on. As @dcporter points out, we have to have unit tests to accompany all the code. If you're unable to do that, we will leave this open until someone else has a chance to do so.

Thanks for your patience.

@nicolasbadia

This comment has been minimized.

Copy link
Member Author

nicolasbadia commented Jun 15, 2012

I'm using this code in my app since about a year but I've never write unit test for it. I will try to write them as soon as I have so time...

@dcporter

This comment has been minimized.

Copy link
Member

dcporter commented Jun 16, 2012

Awesome! Yeah unit tests have sort of an unexpected learning curve, since you set things up a little differently. (There's a guide on sproutcore.com I believe that goes over the basics.) The most important thing is to go in knowing what you're gonna test; in this case I'd suggest one test simulating a keystroke that results in a valid date, and making sure the control's value has updated, and one test simulating a keystroke that results in an INvalid date, and making sure that the value doesn't update.

If you need any help, hit us up.

@nicolasbadia

This comment has been minimized.

Copy link
Member Author

nicolasbadia commented Oct 5, 2012

I've try to add unit tests for this but all the tests using SC.Event are commented and broken (/Users/nicolasbadia/Sites/gestixi/gestixi/frameworks/sproutcore/frameworks/desktop/tests/views/date_field/ui.js).

Since the best way to test what I've done is to use SC.Event, I'd like to know if somebody can help me to fix this commented tests. I've tried with no success.

@dcporter

This comment has been minimized.

Copy link
Member

dcporter commented Oct 29, 2012

This is an awesome feature which just needs some unit test love to be completed. I'm bumping for attention; if nobody has any free time to look at it, I should be able to contribute in the next couple of weeks.

nicolasbadia added some commits Oct 30, 2012

Full review of the code
Fix to a bug with the %Y key.
Allow the use of / - : and space to move to the next tab.
Full review of the code
Fix a bug with the %Y key.
Allow the use of / - : and space to move to the next tab.
Add unit tests
And fix an issue with the years without century.
@nicolasbadia

This comment has been minimized.

Copy link
Member Author

nicolasbadia commented Feb 1, 2013

Just in case you missed it, I've add unit test for this a while ago...

@dcporter

This comment has been minimized.

Copy link
Member

dcporter commented Aug 5, 2013

I thought I remembered seeing this go by... I'm going to pull this into my project tomorrow and give it a proper look.

Edit: and by tomorrow I mean next time I'm not on a deadline with a hard problem.

@mauritslamers

This comment has been minimized.

Copy link
Member

mauritslamers commented Sep 5, 2013

What needs to be done to get this pulled in? Are there still unit tests missing, or is the mentioned SC.Event problem prohibiting completing the tests?

@dcporter

This comment has been minimized.

Copy link
Member

dcporter commented Sep 5, 2013

Main thing it needs is a couple more people to bang on it in production. I promised that I'd pull it into my project, but haven't yet, so I'll get on that list next week.

@dcporter

This comment has been minimized.

Copy link
Member

dcporter commented Sep 11, 2013

I monkeypatched this file into my project and got an error on first keypress – a non-existent global-scope function called "isNumber" is called repeatedly throughout the patch. @nicolasbadia any idea what's going on here?

@nicolasbadia

This comment has been minimized.

Copy link
Member Author

nicolasbadia commented Sep 11, 2013

Yeah, In my first attempt to add support for freeform number entry, I'd forgotten to replace isNumber which was a function that I had created for my app.
But this is fixed by the commit Full review of the code which totally changes the way how freeform number entry are handle.

@dcporter

This comment has been minimized.

Copy link
Member

dcporter commented Sep 11, 2013

So what's going on is I snagged from the wrong commit. It was late, sorry about that. =)

charCode should has been used to get the key code
keyCode is not defined in Firefox.
@dcporter

This comment has been minimized.

Copy link
Member

dcporter commented Dec 6, 2013

I see unit tests failing. Can you take a look at this next time you have a minute?

@nicolasbadia

This comment has been minimized.

Copy link
Member Author

nicolasbadia commented Dec 6, 2013

That damn Radix parameter...
I've opened a new PR. This one is a bit messy...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment