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

Jump on next segment when pressing separator key #26

Merged
merged 10 commits into from
Apr 12, 2016

Conversation

MartinNuc
Copy link
Contributor

Related issue: #25

This PR adds option to jump on the next segment when pressing separator key. For example when filling month in format: 5.7.2016 you can press keys: 5 . 7 . and 2016

Still in progress. To-Do:

  • write tests
  • fill leading zeros when format is HH:mm:ss - when pressing 4 and : on minute segment it should fill 04 and jump on seconds

@MartinNuc MartinNuc changed the title Jump on separator press Jump on next segment when pressing separator key Apr 6, 2016
@MartinNuc
Copy link
Contributor Author

@eight04 Dont know how to write tests for this :-( I have problem with triggering user input. Keyboard events are not exactly what I need to simulate an user input.

Also I am not sure how to add leading zeros. I would have to change date before validation somehow. Do you have any idea?

Is it ok to merge this PR in this state?

@eight04
Copy link
Owner

eight04 commented Apr 6, 2016

I think it's pretty hard to test. You can try:

  1. Focus the element. The directive should create the selection on the first node.
  2. Send keydown
  3. Send keypress
  4. Replace the selection with the key character.
  5. Send keyup.

fill leading zeros when format is HH:mm:ss - when pressing 4 and : on minute segment it should fill 04 and jump on seconds

This seems confusing for me. Is there any other datetime widget following this behavior?

Is it ok to merge this PR in this state?

👍

@eight04
Copy link
Owner

eight04 commented Apr 6, 2016

Wait, I got it.

I think we have to cache NUMBER_TOOSHORT error:
https://github.com/eight04/angular-datetime/blob/master/src/directive.js#L255

Then parser.nodeParseValue here:
https://github.com/eight04/angular-datetime/pull/26/files#diff-c4447b04d8f41855d00d9e821c25ed7cR455

Adding a properValue while throwing the error would be nice too:
https://github.com/eight04/angular-datetime/blob/master/src/factory.js#L512

@MartinNuc MartinNuc force-pushed the jump-on-separator-press branch from ef76982 to 7913cf0 Compare April 11, 2016 09:49
@MartinNuc
Copy link
Contributor Author

Got it working but I am not entirely sure if its done the right way regarding style :-)

Basically when $parser finds error TOO_SHORT it adds suggestedValue to the node which I use later but only when jumping to the next field.

Still having problem with writing test :( focusing input doesn't make a selection. If you want to try out I made a separate branch for it: https://github.com/MartinNuc/angular-datetime/tree/jump-on-separator-press-test

@eight04
Copy link
Owner

eight04 commented Apr 11, 2016

A cleaner way: 957b6f4
Maybe this behavior can also be applied to left/right arrow keys and other errors having .properValue.

Would you like to implement it? or I will merge 12783e7

@eight04
Copy link
Owner

eight04 commented Apr 11, 2016

ariya/phantomjs#12493

@MartinNuc
Copy link
Contributor Author

I will try to implement your way. I wanted to avoid global variable so i put error basically in the node itself. Let me try it :-) and thanks for help.

@MartinNuc
Copy link
Contributor Author

Indeed much cleaner solution :-)

Tried tests in Chrome but its the same :-( sel.rangeCount is zero. This could mean we have problem with focusing input which lead me to this https://groups.google.com/forum/#!topic/angular/dyALxfv8p-U ... tried attaching compile result to the body but still behaved same.

If you are fine that its not tested feel free to merge this PR.

@eight04
Copy link
Owner

eight04 commented Apr 12, 2016

👍

@eight04 eight04 merged commit 1e28dab into eight04:master Apr 12, 2016
@MartinNuc MartinNuc deleted the jump-on-separator-press branch April 12, 2016 21:38
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