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

Move DatePicker and TimePicker to own Component #18087

Closed
wants to merge 4 commits into from

Conversation

jpbelo
Copy link

@jpbelo jpbelo commented Oct 23, 2019

Description

Moves DatePicker and TimePicker to their own Component.
Adds stories for DateTime, Date and Time pickers.
Adds webpack-cli to dev dependencies. (not sure if the scope of the issue allows this)

How has this been tested?

The tests and functionality are the same as before. As the components work as expected on storybook I assumed the tests will pass.
I was not able to run tests locally.

Types of changes

Code improvement

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.

Closes #18072

@jpbelo
Copy link
Author

jpbelo commented Oct 23, 2019

Maybe I should have mentioned it, but there's a problem when you first download the repository, install, and run yarn dev. There's a prompt asking yes/no for installing webpack-cli that then freezes. Adding to the dev dependencies fixes this.

@gziolo gziolo added First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Feature] UI Components Impacts or related to the UI component system [Package] Components /packages/components labels Oct 24, 2019
@jpbelo
Copy link
Author

jpbelo commented Oct 24, 2019

@gziolo can this be labeled with hacktoberfest?

@gziolo
Copy link
Member

gziolo commented Oct 24, 2019

@gziolo can this be labeled with hacktoberfest?

I asked in #core-editor channel on WordPress Slack:
https://wordpress.slack.com/archives/C02QB2JS7/p1571918993111000

I would recommend splitting this PR at least into two parts:

  • extraction of TimePicker component
  • extraction of DatePicker component

This PR is big enough. @mkaz does it make sense? I don't know exactly what did you envision with the original issue.

@mkaz
Copy link
Member

mkaz commented Oct 25, 2019

@mkaz does it make sense? I don't know exactly what did you envision with the original issue.

Just breaking it up so it isn't one directory exporting 3 different component. It currently has a single readme for the three components and they all don't share the same props.

@gziolo
Copy link
Member

gziolo commented Oct 28, 2019

@mkaz does it make sense? I don't know exactly what did you envision with the original issue.

Just breaking it up so it isn't one directory exporting 3 different component. It currently has a single readme for the three components and they all don't share the same props.

I see. It makes a lot of sense. Do you think they should be moved to the top-level directory? The only challenge is that the following code would have to be imported for all of them:

// Needed to initialise the default datepicker styles.
// See: https://github.com/airbnb/react-dates#initialize
import 'react-dates/initialize';

or moved to the top-level index.js.

@mkaz
Copy link
Member

mkaz commented Oct 28, 2019

I don't know enough about what the react-dates/initialize does, if it sets any "now" times or something similar then it should be done top level so all components have the same initialization. I can see weird race conditions happening at midnight. So I would need to read up more on that.

Personally I'd preer the code to be in each component, since if someone is looking at using just the DatePicker or just the TimePicker and review the code they can see quickly from its source where it is initialized from.

Base automatically changed from master to trunk March 1, 2021 15:42
@t-hamano
Copy link
Contributor

Hi @jpbelo,

Sorry it took me so long to reply to you.

I have checked the latest trunk and the three components are split into their own components and there is also Storybook. However, the README and Component Reference only exists for the DateTimePicker component.

It might be nice to have separate READMEs for TimePicker and DatePicker, like the subcomponents of the Card component.

This PR has the README for the above two components. Therefore, how about rebasing this PR to the latest trunk and keeping only the two READMEs?

If you don't have time to work on it, I will gladly take it over 👍

@youknowriad
Copy link
Contributor

Looks like there's no activity here anymore. I'm closing this PR. Feel free to open a new one if these efforts are to be restored since it needs to be redone entirely.

Thanks for your efforts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Package] Components /packages/components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add READMEs for DatePicker and TimePicker
5 participants