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

Add READMEs for DatePicker and TimePicker #18072

Open
mkaz opened this issue Oct 22, 2019 · 10 comments
Open

Add READMEs for DatePicker and TimePicker #18072

mkaz opened this issue Oct 22, 2019 · 10 comments
Labels
Good First Issue An issue that's suitable for someone looking to contribute for the first time [Package] Components /packages/components [Type] Developer Documentation Documentation for developers

Comments

@mkaz
Copy link
Member

mkaz commented Oct 22, 2019

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

@mkaz mkaz added Good First Issue An issue that's suitable for someone looking to contribute for the first time [Package] Components /packages/components labels Oct 22, 2019
@jpbelo
Copy link

jpbelo commented Oct 22, 2019

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

@ItsJonQ
Copy link

ItsJonQ commented Oct 23, 2019

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

@jpbelo
Copy link

jpbelo commented Oct 23, 2019

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
Copy link
Member

gziolo commented Oct 23, 2019

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
Copy link

jpbelo commented Oct 23, 2019

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

@mkaz
Copy link
Member Author

mkaz commented Oct 24, 2019

@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
Copy link

jpbelo commented Oct 25, 2019

@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
Copy link
Member

gziolo commented Oct 28, 2019

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:

export { NavigableMenu, TabbableContainer } from './navigable-container';

export { createSlotFill, Slot, Fill, Provider as SlotFillProvider } from './slot-fill';

In fact, we are exposing 3 components at the moment, what do we miss?:

export { DateTimePicker, DatePicker, TimePicker } from './date-time';

@t-hamano
Copy link
Contributor

t-hamano commented Apr 9, 2023

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:

export { default as DateTimePicker, DatePicker, TimePicker } from './date-time';

Storybook is also available:

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

@stokesman
Copy link
Contributor

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

@jordesign jordesign added the [Type] Enhancement A suggestion for improvement. label Aug 16, 2023
@mirka mirka added [Type] Developer Documentation Documentation for developers and removed [Type] Enhancement A suggestion for improvement. labels Mar 19, 2024
@mirka mirka changed the title Move DatePicker and TimePicker to own Component Add READMEs for DatePicker and TimePicker Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue An issue that's suitable for someone looking to contribute for the first time [Package] Components /packages/components [Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants