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

feat(timepicker): input filter to accept only numbers #3247

Merged
merged 2 commits into from Nov 15, 2019

Conversation

fbasso
Copy link
Member

@fbasso fbasso commented Jun 21, 2019

Finally a better proposal than #3236 to fix #2334

  • The timepicker inputs are filtered (so a better approach than type="number" which supported differently depending on the browsers),
  • inputmode="numeric" is used to open the numeric keypad on mobile device, which is basically the purpose of this attribute, for the supported browsers (https://caniuse.com/#feat=input-inputmode : Chrome, Opera, iOs safari, Android, Samsung Internet)

@benouat
Copy link
Member

benouat commented Jun 24, 2019

@IAfanasovMob Thanks for the review. It is much appreciated here!

Copy link
Member

@benouat benouat left a comment

Choose a reason for hiding this comment

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

@fbasso Would you mind write some unit tests and/or some e2e tests for that?

@codecov-io
Copy link

codecov-io commented Jun 24, 2019

Codecov Report

Merging #3247 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3247      +/-   ##
==========================================
+ Coverage   91.12%   91.12%   +<.01%     
==========================================
  Files          95       95              
  Lines        2793     2795       +2     
  Branches      516      516              
==========================================
+ Hits         2545     2547       +2     
  Misses        190      190              
  Partials       58       58
Flag Coverage Δ
#e2e 56.2% <100%> (+0.38%) ⬆️
#unit 87.96% <50%> (-0.03%) ⬇️
Impacted Files Coverage Δ
src/timepicker/timepicker.ts 98.66% <100%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4d3848...ca96eac. Read the comment docs.

const metaKey = e.metaKey;
const shiftKey = e.shiftKey;
if ([46, 8, 9, 27, 13].indexOf(keyCode) !== -1 || // Allow: Delete, Backspace, Tab, Escape, Enter
(keyCode === 65 && ctrlKey === true) || // Allow: Ctrl+A
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if this is any better but can also do something like

([65, 67, 86, 88].indexOf(keyCode) !== -1  && (ctrlKey === true || metaKey === true)) ||    // Allow: Ctrl+A

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @msosa, yes you're right. I took this code somewhere on internet, but never had a closer look at the "if" part (it was the more complete I found, taking account of such cases). I'm going to change it.

@benouat benouat modified the milestones: 5.0, 5.1.0 Jun 26, 2019
@benouat benouat modified the milestones: 5.1.0, 5.1.1 Jul 17, 2019
@fbasso fbasso force-pushed the timepicker-filter branch 2 times, most recently from 2581725 to 300348c Compare September 3, 2019 09:02
@maxokorokov maxokorokov modified the milestones: 5.1.2, 5.2 Sep 3, 2019
Copy link
Member

@benouat benouat left a comment

Choose a reason for hiding this comment

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

I am fine with these changes, but questioning myself if we should land this PR or not...

Copy link
Member

@maxokorokov maxokorokov left a comment

Choose a reason for hiding this comment

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

Hey @fbasso,

let's replace all the keycode magic with a simple regexp magic as @benouat suggested

@fbasso
Copy link
Member Author

fbasso commented Nov 15, 2019

Done !

Copy link
Member

@maxokorokov maxokorokov left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@fbasso I took the liberty of refactoring / simplifying it a bit, pushed a commit. Please let me know if it is fine with you?

  • Uppercase const name
  • _format(event)formatInput(inputElement)

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

Successfully merging this pull request may close these issues.

TimePicker - input type number
6 participants