Redo TimeInput #151

dferber90 · 2018-10-08T12:09:14Z

Recreates TimeInput without flatpickr.

Advantages of new `TimeInput`

UX:

shows time depending on locale
allows user to enter time using the keyboard
it is possible to enter time in the 24h format or 12h format

Code improvements:

does not use flatpicker anymore, so it’s A LOT cheaper to initialize (I dropped all optimizations we made, where initialized it on click only, and not at all on mobile)
renamed isInvalid to hasError as we use that for all inputs
added isAutofocussed
removed unused mode prop
removed unused timeZone prop
value can now always be converted to 24h format using TimeInput.to24h(value)
added name for easier use with Formik

Problems of old `TimeInput`

It has the UX limitations that:

~~it is not possible to enter any value with the keyboard~~
no matter the locale, the value is always entered and shown in 12hr format (AM/PM)
The documentation was wrong
~~users can only adjust the time by a specificity of 5 minutes~~
users can not specify time more precise than minutes even though the API supports precision up to milliseconds

Technically it had the problems that:

It had support for props that had no effect on it or even broke it when unsupported props were passed
it is really heavy to initialize so we had to defer initialization for Modified Products where many TimeInput fields can be present at the same time

dferber90 · 2018-10-08T12:16:55Z

Tests are failing on CI because of something related to locale apparently. The tests are succeeding on my machine. I'm investigating.

dferber90 · 2018-10-08T14:28:18Z

Fixed the failing test. Ready for review now :)

dferber90 · 2018-10-08T14:33:47Z

Ok, that wasn't very long until a new requirement surfaced.

The input should support a higher precision than currently shown. It should support up to 14:00:00.000, as the API supports the same precision.

I'll add this before putting the component into review again.

dferber90 · 2018-10-08T18:11:58Z

The TimeInput now supports high precision times as well. It will only show the high-precision time (eg 14:00:05.895) when seconds or milliseconds are entered. Otherwise it will show the basic time (eg 14:00).

dferber90 · 2018-10-09T09:06:09Z

High precision timing for ya'll

montezume · 2018-10-09T09:31:53Z

@dferber90 so much precision, so much wow 😮 👍

antonboyko

@dferber90 good job!

tdeekens

Super nice too see so much dependency cruft gone. Thanks a lot! Just left some thoughts.

tdeekens · 2018-10-09T11:11:40Z

src/components/inputs/time-input/time-input-body.js

+
+export const ClearSection = props => (
+  <div
+    onClick={props.isDisabled ? undefined : props.onClear}


I've seen null being passed in that case. I don't mind but was wondering if there is a specific reason.

I'm not aware of a difference and don't have any arguments for either side. I usually use undefined though.

If the target component was a custom component (not a "native" one like a div), then passing null would lead to the default prop not overwriting it if I'm not mistaken. Hence, I usually use undefined. In this case it does not matter though.

src/components/inputs/time-input/time-input-body.js

src/components/inputs/time-input/time-input.spec.js

tdeekens · 2018-10-09T11:14:31Z

src/components/inputs/time-input/time-input.spec.js

+
+  it('should have focus automatically when isAutofocussed is passed', () => {
+    const { container } = render(<TimeInput {...baseProps} isAutofocussed />);
+    expect(container.querySelector('input')).toHaveFocus();


dferber90

Noticed some things myself

src/components/inputs/time-input/time-input.js

dferber90 · 2018-10-10T07:08:47Z

This was approved by @coolmilo as well, so we're good to go!

I'm thinking of waiting with the merge though, so that DateInput, DateTimeInput and TimeInput can be released at once.

dferber90 · 2018-10-14T11:44:47Z

src/components/inputs/time-input/time-input.js

+    hours: Number(hours) + (amPm === 'pm' ? 12 : 0),
+    minutes: Number(minutes),
+    seconds: Number(seconds),
+    milliseconds: Number(milliseconds),


The parsing of milliseconds is actually wrong. When a user enters 00:00:00.75 we parse 75 milliseconds, but it should be 750. We need to use right-pad to fill up to 3 digits with 0s before parsing the number.

The bug isn't noticeable because we use rightPad before displaying the data in format24hr, but the parseTime function itself is broken.

emmenko · 2018-10-25T08:00:03Z

@dferber90 anything else blocking this? Can you give us a status update on how to proceed here?

dferber90 · 2018-10-25T08:03:27Z

See the last comment:

I'm thinking of waiting with the merge though, so that DateInput, DateTimeInput and TimeInput can be released at once.

I want to merge this along with #166 so that they can be released at the same time. That's also why the "Awaiting release" label is on this PR. Otherwise consumers would have to do the migration in two steps if we release in between because of a bugfix or something alike.

#166 is still missing the implementation of the new designs, but I've started that yesterday.

emmenko · 2018-10-26T09:23:42Z

Ok thanks for the update 👌

Reimagine the component and drop flatpickr.

Workaround for the node Intl API (not react-intl itself). Had to stop using props.intl.formatTime as it was not respecting the passed locale when formatting time.

And change cursor to default one for the input.

Yeah, I know it's bad. The other inputs do not use an outline either right now. There is alternative styling instead. We are discussiong to readd outlines at once eventually.

dferber90 added 💅 Type: Enhancement Improves existing code 🙏Status: Dev Review 👨‍🎨 Status: UI/UX Review Requires review from design team labels Oct 8, 2018

dferber90 removed the 🙏Status: Dev Review label Oct 8, 2018

dferber90 force-pushed the df-redo-time-input branch 2 times, most recently from 39d99cd to 28a8d0c Compare October 8, 2018 14:01

dferber90 added the 🙏Status: Dev Review label Oct 8, 2018

dferber90 removed the 🙏Status: Dev Review label Oct 8, 2018

dferber90 added 🙏Status: Dev Review 👨‍🎨 Status: UI/UX Review Requires review from design team and removed 👨‍🎨 Status: UI/UX Review Requires review from design team labels Oct 8, 2018

dferber90 requested a review from lufego October 9, 2018 08:33

dferber90 requested a review from antonboyko October 9, 2018 09:52

antonboyko approved these changes Oct 9, 2018

View reviewed changes

tdeekens reviewed Oct 9, 2018

View reviewed changes

dferber90 commented Oct 9, 2018

View reviewed changes

src/components/inputs/time-input/time-input.js Show resolved Hide resolved

src/components/inputs/time-input/time-input.js Outdated Show resolved Hide resolved

src/components/inputs/time-input/time-input.js Outdated Show resolved Hide resolved

tdeekens approved these changes Oct 9, 2018

View reviewed changes

dferber90 added ⏲Status: Awaiting release and removed 👨‍🎨 Status: UI/UX Review Requires review from design team 🙏 Status: Dev Review labels Oct 10, 2018

dferber90 commented Oct 14, 2018

View reviewed changes

dferber90 force-pushed the df-redo-time-input branch 2 times, most recently from d861157 to e01cead Compare October 17, 2018 14:06

dferber90 force-pushed the df-redo-time-input branch from e01cead to ed5acdc Compare October 17, 2018 14:19

dferber90 removed the request for review from lufego October 22, 2018 13:09

dferber90 force-pushed the df-redo-time-input branch from ed5acdc to c3b5fbd Compare October 24, 2018 14:44

dferber90 force-pushed the df-redo-time-input branch from 2d33e5a to 66607f0 Compare November 5, 2018 09:44

dferber90 force-pushed the df-redo-time-input branch from 289c300 to f2d5b1b Compare November 13, 2018 09:56

dferber90 added 14 commits December 5, 2018 12:08

test(test-utils): add abilitiy to set locale

0e684ab

feat(time-input): redo component

3123077

Reimagine the component and drop flatpickr.

test(time-input): work around Intl API

4554567

Workaround for the node Intl API (not react-intl itself). Had to stop using props.intl.formatTime as it was not respecting the passed locale when formatting time.

feat(time-input): support high precision

8e57786

refactor(time-input): simplify high precision handling

a6df829

docs(time-input): explain form usage

77ab3dd

feat(time-input): make clock icon a label

2bc38b7

And change cursor to default one for the input.

test(time-input): group TimeInput.to24h tests

b7c15ab

style(time-input): remove outline

ab7ddfc

Yeah, I know it's bad. The other inputs do not use an outline either right now. There is alternative styling instead. We are discussiong to readd outlines at once eventually.

fix(time-input): fix milliseconds parsing

44a8659

refactor(time-input): use default export for time-input-body

6e50ef4

fix(time-input): avoid 12am/12pm jump when blurring repeatedly

fe5248b

fix(time-input): format time when blurred

caf85d5

fix(time-input): do not allow hours>12 for 12h clock

d10c881

dferber90 force-pushed the df-redo-time-input branch from f2d5b1b to d10c881 Compare December 5, 2018 11:09

dferber90 merged commit 4d0205e into master Dec 5, 2018

dferber90 deleted the df-redo-time-input branch December 5, 2018 11:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redo TimeInput #151

Redo TimeInput #151

dferber90 commented Oct 8, 2018 •

edited

Loading

dferber90 commented Oct 8, 2018

dferber90 commented Oct 8, 2018

dferber90 commented Oct 8, 2018

dferber90 commented Oct 8, 2018

dferber90 commented Oct 9, 2018

montezume commented Oct 9, 2018

antonboyko left a comment

tdeekens left a comment

tdeekens Oct 9, 2018

dferber90 Oct 9, 2018

tdeekens Oct 9, 2018

dferber90 left a comment

dferber90 commented Oct 10, 2018 •

edited

Loading

dferber90 Oct 14, 2018

emmenko commented Oct 25, 2018

dferber90 commented Oct 25, 2018

emmenko commented Oct 26, 2018

Redo TimeInput #151

Redo TimeInput #151

Conversation

dferber90 commented Oct 8, 2018 • edited Loading

Advantages of new TimeInput

Problems of old TimeInput

dferber90 commented Oct 8, 2018

dferber90 commented Oct 8, 2018

dferber90 commented Oct 8, 2018

dferber90 commented Oct 8, 2018

dferber90 commented Oct 9, 2018

montezume commented Oct 9, 2018

antonboyko left a comment

Choose a reason for hiding this comment

tdeekens left a comment

Choose a reason for hiding this comment

tdeekens Oct 9, 2018

Choose a reason for hiding this comment

dferber90 Oct 9, 2018

Choose a reason for hiding this comment

tdeekens Oct 9, 2018

Choose a reason for hiding this comment

dferber90 left a comment

Choose a reason for hiding this comment

dferber90 commented Oct 10, 2018 • edited Loading

dferber90 Oct 14, 2018

Choose a reason for hiding this comment

emmenko commented Oct 25, 2018

dferber90 commented Oct 25, 2018

emmenko commented Oct 26, 2018

dferber90 commented Oct 8, 2018 •

edited

Loading

Advantages of new `TimeInput`

Problems of old `TimeInput`

dferber90 commented Oct 10, 2018 •

edited

Loading