Skip to content

Improved handling of leading zeros, length limit and UX #115

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

Closed
wants to merge 4 commits into from

Conversation

marcoancona
Copy link

Please consider these changes. I don't see disadvantages, but maybe you see some.

Advantages:

  • no extra span element for leading zeros (which is difficult to keep aligned to the rest) and makes selection weird
  • keep expected behavior for showLeadingZeros
  • ensures the user does not type more than 2 digits (which is always invalid day)
  • the user can type a sequence of numbers, just the last two will be used (this feels like the most natural behavior for this kind of field to me)

I only made this changes to the DayInput, but I could also extend to the other inputs if we agree this makes sense.

Marco Ancona added 3 commits November 2, 2018 17:05
Advantages:
- no extra span element for leading zeros (which is difficult to keep aligned to the rest) and makes selection weird
- keep expected behavior for `showLeadingZeros`
- ensures the user does not type more than 2 digits (which is always invalid day)
- the user can type a sequence of numbers, just the last two will be used (this feels like the most natural behavior for this kind of field to me)

I only made this changes to the DatInput, but I could also extend to the other inputs if we agree this makes sense.
@marcoancona
Copy link
Author

I didn't check the tests and the code style. I can do this once I make the changes on all inputs.

@wojtekmaj
Copy link
Owner

Hey,
thank you for your contribution.

But, oh, if it was so simple! :) That solution was the first thing that came to my mind. On Chrome it works as expected, but Firefox cuts the leading zeros from number inputs. Because of that I was forced to create this leading zero outside of the input. I didn't want to create differences between how it is handled on different browsers, too, because this would lead to many implementation bugs.

It's a good idea to limit the input length, though. In fact, typing two digits in native date picker moves the focus to the next field, so this is how it should be done. It's on my list now!

I regret that I need to close this. Thanks again!

@wojtekmaj wojtekmaj closed this Nov 5, 2018
@marcoancona
Copy link
Author

marcoancona commented Nov 5, 2018

I see, I work on Electron and always forget about cross-browser compatibility 😅
So what about using “normal” input fields instead of “number”? My solution would work with that, wouldn’t it? What are the drawbacks? I see that you need to listen for ArrowUp/ArrowDown and increase/decrease the value manually, but that is easy.

PS: Sorry if I bother, I think this is already the best solution for this kind of component but still I would like to help make it at the level of some native pickers. Right now, on Chrome, if the user types ‘0x’ and leading zero is on, the field shows ‘00x’ which is not good. On the other hand, I would not go down the road of conditionally check the browser name.

@wojtekmaj
Copy link
Owner

Using text input would:

  • Remove numeric touch keyboard from the input (this is the biggest one)
  • Remove the ability to increment/decrement values if not implemented
  • Remove input pre-validation which allows digits only

I do realize that the way it works right now is not ideal, and will work on that to improve in the near future.

@marcoancona
Copy link
Author

About 2) and 3) I provide an implementation in #116
I like, in particular, the behavior of increase/decrease if handled at date level, instead of single component level.
About 1), I guess you can force the right keyboard with a combination of inputmode and pattern attributes but I have not tried.

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