time-picker overview - Githubissues

wix-playground / stylable-components

Fully-tested & strictly-typed component library based on React, using optional Wix styling.

MIT License

35 stars 3 forks source link

time-picker overview #364

Closed nadavwix closed 7 years ago

nadavwix commented 7 years ago

docs

[x] remove rtl, should be implemented using context
[x] enum ('ampm’, '24hr’) change to "ampm" | "24hr" this is a valid TS syntax for enum and more readable
[x] children should be separated to prefix and suffix props
[x] keyboard behavior is unclear in spec and missing delete/bkspace
[x] RTL behavior is missing

implementation

[x] missing properties decorator ~~- [ ] AM/PM button should not switch sides in RTL mode~~
[x] rtl should be implemented via context
[x] change AMPM enum to "AM" | "PM" typings format
[x] segment data should be removed from state as it is derived from props
[x] native input support should be replaced with mobile detection per guidlines this should support mocking in tests

style

[x] change camel case classNames

tests

[x] entering "5" into mm input and entering "7" into mm input seems oddly repitative.
[x] most describes in this file should be "it"s
[x] tests should say what they test and not what the test does.

maksymc commented 7 years ago

"AM/PM button should not switch sides in RTL mode"

It should. Most of the resources that we've checked do that: ~ Booking.com ~ http://demos.telerik.com/kendo-ui/timepicker/right-to-left-support ~ native in Chrome & FF (version 57)

maksymc commented 7 years ago

"keyboard behavior is unclear in spec and missing delete/bkspace"

Added delete/bkspace behavior
As for "keyboard behavior is unclear" I am not sure what exactly is wrong. yes, it is complex but 70% of stuff is taken from w3 aria specs, so it should be pretty clear. Let me know if there is anything specific that is hard for you to understand.

wtfil commented 7 years ago

@nadavwix @properties, Ampm are fixed at https://github.com/wix/stylable-components/pull/458 rtl already implemented via context

Regarding mobile detection - we can not use mobile detection as is, because there are some mobile devices (Lumia) which are detected as touch devices (which is true) but they did not have the <input type='time'/> input. So I extends touch detection for this component to use native input only on touch devices which can actually show it

wtfil commented 7 years ago

@nadavov all tests for timepicker has beed refactored at https://github.com/wix/stylable-components/pull/466/files No more copy/paste and better tests titles

maksymc commented 7 years ago

segment data should be removed from state as it is derived from props

According to Fill this item is no longer relevant. Marking as done & closing the ticket.