WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.52k stars 4.2k forks source link

Add READMEs for DatePicker and TimePicker #18072

Open mkaz opened 5 years ago

mkaz commented 5 years ago

See #17497

The DatePicker and TimePicker components are included as part of the DateTimePicker component, they should be split out to their own components, with their own README and Story files.

Since they were previously included as part of the DateTimePicker component, to avoid breaking backward compatibility, they should be imported & exported properly in the old component

jpbelo commented 5 years ago

Can I pick this one? It's my first time contributing to open source, but I think I can do this properly.

ItsJonQ commented 5 years ago

@jpbelo Awesome!! Go for it :D. Excited to see your pull request!

jpbelo commented 5 years ago

I eventually sorted it out by installing webpack-cli manually. But when you have a fresh clone and run yarn and then yarn dev, you will be prompted to install webpack-cli (yes/no). It then gets stuck there. Maybe there's a bug in.

gziolo commented 5 years ago

Since they were previously included as part of the DateTimePicker component, to avoid breaking backward compatibility, they should be imported & exported properly in the old component

@mkaz - do you propose to deprecate the old one with a note for developers to use new components instead? I'm fine with that, I just wanted to make sure it's clear.

jpbelo commented 5 years ago

Can someone give me help here? I’m not sure why travis failed...

mkaz commented 5 years ago

@gziolo I'm not quite sure how to deprecate, or if it is actually needed. Now that I talk it out, maybe it won't be an issue. The comment (also it was originally from @ItsJonQ , but I feel the same) is basically to not break anyone's code that might of previous done something like:

import { DatePicker } from '/date-time';

So inside of date-time/index.js we could include

import { DatePicker} from '../date-picker';
export { DatePicker };

Thinking about it, I don't think externally people import using ./date-time they do so using the package @wordpress/components like so

import { DatePicker } from '@wordpress/components';

So just updating the components/src/index.js to pull from the correct spot should be sufficient, right?

jpbelo commented 5 years ago

@mkaz I think you are correct on how external users import from @wordpress/components.

Regarding the import/export from date-picker, I think I covered that here: https://github.com/WordPress/gutenberg/pull/18087/files#diff-52f4edceeb4c11275063e5868f3c66ff

gziolo commented 5 years ago

Yes, it's totally fine to have 3 components exposed. We can also put all of them in their own files if that helps. We don't have strict rules for exports from the @wordpress/components package, see some examples:

https://github.com/WordPress/gutenberg/blob/711f9ff1ee44208e3f18da441637db59e6891267/packages/components/src/index.js#L37 https://github.com/WordPress/gutenberg/blob/711f9ff1ee44208e3f18da441637db59e6891267/packages/components/src/index.js#L67

In fact, we are exposing 3 components at the moment, what do we miss?: https://github.com/WordPress/gutenberg/blob/711f9ff1ee44208e3f18da441637db59e6891267/packages/components/src/index.js#L14

t-hamano commented 1 year ago

I noticed that this problem may have been solved in the latest trunk. The three components DateTimePicker, DatePicker, and TimePicker appear to be exported independently:

https://github.com/WordPress/gutenberg/blob/7810f7bdc29f3ba798aee28e5c70c97d8825fda0/packages/components/src/index.ts#L64

Storybook is also available:

Can we close this issue and the related PR #18087?

stokesman commented 1 year ago

If it's important that they have their own README files then this is not yet done. Otherwise, looks like we could close it.