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

Fix not truncating at minutes the first time in <TimePicker> #24305

Merged

Conversation

kevin940726
Copy link
Member

Description

Continued from #15495.

Seems like that PR only fixes truncating at minutes the second time and beyond. When first calling changeDate, the truncated date is saved in the Component's internal state, but not passed as a argument of onChange.

The detailed steps are as follow:

  1. <TimePicker> initializes the date with currentTime or now. which contains seconds like 2020-07-31T06:45:35.
  2. User updates the time, e.g. updating minutes from 45 to 46, and blur the input.
  3. changeDate updates state with zeroed seconds (2020-07-31T06:46:00)
  4. However, onChange is triggered with non-zeroed seconds (2020-07-31T06:46:35)
  5. <TimePicker> re-renders using the new state as the new date
  6. User updates the time again, e.g. updating hours from 06 to 07.
  7. changeDate triggered, and because state was updated with zeroed seconds, newDate now also has zeroed seconds (2020-07-31T07:46:00)

Note the 4th step, the component will temporary have a non-zeroed second in the date value.

It's likely that users tend to not only update the time once, normally they would update it multiple times, that's why we didn't notice the bug before 🤔 ?

I'm also trying to refactor this component into function component. The updated implementation should not have this bug, but figured to open a bug fix PR first to get it landed. Will open another PR for the refactor.

How has this been tested?

Added unit test in test/time.js.

Also can tested in the <PostSchedule> component. Create a new post and change the schedule time in the settings, and observe the value in devtools or React devtools.

Types of changes

Bug fix

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.

@talldan talldan added [Type] Bug An existing feature does not function as intended General Interface Parts of the UI which don't fall neatly under other labels. labels Jul 31, 2020
Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

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

Great work on spotting this issue and tracing it back to #15495. Thanks for also adding the test!

Fix makes sense, and worked well in my manual testing.

@talldan talldan merged commit ab5eedf into WordPress:master Jul 31, 2020
@github-actions github-actions bot added this to the Gutenberg 8.7 milestone Jul 31, 2020
@kevin940726 kevin940726 deleted the update/time-picker-zero-seconds branch July 31, 2020 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
General Interface Parts of the UI which don't fall neatly under other labels. [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants