Remove TimePicker state and Moment dependency by pablinos · Pull Request #25471 · WordPress/gutenberg

pablinos · 2020-09-19T00:57:48Z

Description

While working on the post schedule feature, @retrofox and I noticed that there are a number of bugs with the implementation of the DateTimePicker. The TimePicker was holding its own local state, and would only update the month when the select component lost focus.

This change updates the TimePicker component to:

Not rely on local state
Use date-fns rather than Moment
Tweak the triggering of the onChange callback.

How has this been tested?

Locally using Storybook

Screenshots

Types of changes

This could be a breaking change, depending on how others are using the TimePicker component.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR.

pablinos · 2020-09-19T17:32:01Z

packages/components/src/date-time/index.js

I'm not sure this default is needed, as it's initialised elsewhere, and should probably be handled by default props instead. This is a reminder to me to remove it!

packages/components/src/date-time/date.js

pablinos · 2020-09-20T11:26:31Z

I'm getting a problem when this is used with our version of the DatePicker based on React DateTimePicker. It has code to grab focus when the selected date changes. This is happening as I tab out of one of the TimePicker's input fields, but strangely not when I hit enter. This is odd as it causes the same update function to be called.

ZebulanStanphill · 2020-09-20T13:58:42Z

packages/components/src/date-time/time.js

Suggested change

import startOfMinute from 'date-fns/startOfMinute';

import set from 'date-fns/set';

import format from 'date-fns/format';

import { format, set, startOfMinute } from 'date-fns';

ZebulanStanphill · 2020-09-20T14:00:10Z

packages/components/src/date-time/time.js

Suggested change

function IntegerValidatedField( { as, value, onUpdate, ...props } ) {

const element = as ?? 'input';

function IntegerValidatedField( { as = 'input', value, onUpdate, ...props } ) {

const element = as;

When using `defaultValue` the change to the DOM node meant that focus was being lost as the `TimePicker` was rendered.

stokesman · 2023-06-09T23:32:05Z

I'd wager that all the problems this PR intended to address are since resolved and the last push here is going to be three years old soon. We should close this one out, no?

pablinos · 2023-06-12T22:50:34Z

I'd wager that all the problems this PR intended to address are since resolved and the last push here is going to be three years old soon. We should close this one out, no?

Yes we should!

pablinos requested review from ajitbohra, chrisvanpatten and youknowriad as code owners September 19, 2020 00:57

pablinos marked this pull request as draft September 19, 2020 00:58

pablinos commented Sep 19, 2020

View reviewed changes

ZebulanStanphill reviewed Sep 19, 2020

View reviewed changes

packages/components/src/date-time/date.js Outdated Show resolved Hide resolved

ZebulanStanphill added the [Type] Bug An existing feature does not function as intended label Sep 19, 2020

pablinos force-pushed the remove/timepicker-state branch from 418b6de to 7ef881a Compare September 20, 2020 10:55

ZebulanStanphill reviewed Sep 20, 2020

View reviewed changes

retrofox added 15 commits September 21, 2020 10:32

packakes: add react-datepicker component

a2ec6ca

packages: add date-fns dependency

8a02e48

date-time: replace react-dates by react-datepicker

7116685

date-time: handling onChange

ee89f05

base-styles: define $gray-800

cd1a92e

date-time: adjust month view styles

6865dbc

date-time: restore TimePicker component

de93f16

date-time: add DatePicker to storybook

32966af

date-time: simplify storybook

f7b9a74

date-time: improve date picker header

2c7d7e6

date-time: improve date storybook. add set today

6c762b3

date-time: handling locale

f790158

date-time: start to mark events in calendar

1a8ed5e

date-time: starting to work on event tooltips

bda2b41

date-time: limit events per day

36bf38c

retrofox force-pushed the update/post-schedule-switch-calendar-component branch from 5561271 to 36bf38c Compare September 21, 2020 15:07

retrofox added 3 commits September 21, 2020 12:11

post-schedule: remove console.log

a44acec

date-time: highlight site posts

fc2b5b4

post-schedule: pick up events filtering by month

c385d84

retrofox and others added 13 commits September 21, 2020 12:17

post-schedule: replace HOC by hooks

acada5f

post-schedule: refactor getting events by month

dfc7f0e

post-schedule: rendering site events

6db022d

date-picker: applying styles to events

67243c9

post=schedule: fix popiver button element

f02116d

post-schedule: limit requestng entities

1058257

colors: remove $gray-800 color

1e27038

Make the Timepicker a pure component

fdf0e4d

Add date-fns package

32af699

Update packages, so DateTimePicker still works

3815a49

Use 'element' variable instead of recalculating the value

f53a370

Adapt the DateTimePicker to use the new TimePicker implementation

68c94e8

Use local state for IntegerValidatedField to better handle focus

b757a45

When using `defaultValue` the change to the DOM node meant that focus was being lost as the `TimePicker` was rendered.

pablinos force-pushed the remove/timepicker-state branch from 15c5191 to b757a45 Compare September 21, 2020 20:02

Tidy up imports and fix the linting errors

663af60

ZebulanStanphill added the [Package] Components /packages/components label Sep 22, 2020

Fix the key warning when rendering the Tooltips

6daabbc

retrofox force-pushed the update/post-schedule-switch-calendar-component branch from 3234078 to 58bf061 Compare September 22, 2020 18:34

pablinos closed this Jun 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove TimePicker state and Moment dependency#25471

Remove TimePicker state and Moment dependency#25471
pablinos wants to merge 33 commits intoWordPress:update/post-schedule-switch-calendar-componentfrom
pablinos:remove/timepicker-state

pablinos commented Sep 19, 2020

Uh oh!

pablinos Sep 19, 2020

Uh oh!

Uh oh!

pablinos commented Sep 20, 2020

Uh oh!

ZebulanStanphill Sep 20, 2020

Uh oh!

ZebulanStanphill Sep 20, 2020

Uh oh!

stokesman commented Jun 9, 2023

Uh oh!

pablinos commented Jun 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

-import startOfMinute from 'date-fns/startOfMinute';
-import set from 'date-fns/set';
-import format from 'date-fns/format';
+import { format, set, startOfMinute } from 'date-fns';

-function IntegerValidatedField( { as, value, onUpdate, ...props } ) {
-	const element = as ?? 'input';
+function IntegerValidatedField( { as = 'input', value, onUpdate, ...props } ) {
+	const element = as;

Conversation

pablinos commented Sep 19, 2020

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

Uh oh!

pablinos Sep 19, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pablinos commented Sep 20, 2020

Uh oh!

ZebulanStanphill Sep 20, 2020

Choose a reason for hiding this comment

Uh oh!

ZebulanStanphill Sep 20, 2020

Choose a reason for hiding this comment

Uh oh!

stokesman commented Jun 9, 2023

Uh oh!

pablinos commented Jun 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants