Refactor PostSchedule to make calendar/clock available as components
#4180
Conversation
components/date-time/date.js
Outdated
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { settings } from '@wordpress/date'; |
There was a problem hiding this comment.
I'd prefer if we avoid using the date module in the components module to keep it as generic as possible?
components/date-time/date.js
Outdated
| /** | ||
| * External dependencies | ||
| */ | ||
| import { default as ReactDatePicker } from 'react-datepicker'; |
There was a problem hiding this comment.
Minor: this could be import ReactDatePicker from 'react-datepicker';
There was a problem hiding this comment.
(Note that we may change react-datepicker later as it's not accessible.
components/date-time/index.js
Outdated
| import { DatePicker } from './date'; | ||
| import { TimePicker } from './time'; | ||
|
|
||
| export { DatePicker } from './date'; |
There was a problem hiding this comment.
Since we already imported those, we could drop the from
export { DatePicker, TimePicker }
components/date-time/time.js
Outdated
|
|
||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { __ } from '@wordpress/i18n'; | ||
| import { Button } from '@wordpress/components'; | ||
| import Button from '../button'; | ||
| import { settings } from '@wordpress/date'; |
There was a problem hiding this comment.
Same here, we may avoid this dependency and just add a is12Hour prop instead?
components/date-time/index.js
Outdated
| export { DatePicker } from './date'; | ||
| export { TimePicker } from './time'; | ||
|
|
||
| export function DateTimePicker( { currentDate, onChange, locale, ...args } ) { |
There was a problem hiding this comment.
Maybe this could be exported as "default"
There was a problem hiding this comment.
Exporting the function as default doesn't seem possible; doing so returns undefined when React attempts to render it.
| is12Hour={ is12HourTime } | ||
| />, | ||
| ]; | ||
| return <DateTimePicker |
There was a problem hiding this comment.
In general, for multi-lines elements, we tend to wrap them in () so something like
return (
<Component
prop={ value }
/>
);
|
Thanks for the changes, just one last thing I forgot: we should add "moment" to the dependencies of the "wp-components" script in Also, do you mind adding a |
|
Added a |
|
@travislopes It looks like the unit tests are failing because of You can still run |
components/date-time/time.js
Outdated
| @@ -10,7 +10,7 @@ import moment from 'moment'; | |||
| import { __ } from '@wordpress/i18n'; | |||
| import Button from '../button'; | |||
|
|
|||
| export function TimePicker( { currentTime, onChange, is12Hour, ...args } ) { | |||
There was a problem hiding this comment.
This change makes me wonder if the extra args are really necessary for the DateTimePicker and DatePicker. It may be better if we're explicit here. (for example, explicitly pass the locale prop to the DatePicker
There was a problem hiding this comment.
TimePicker makes 100% sense to remove the extra args from since no other variables are utilized.
DatePicker, and therefore DateTimePicker, however I believe should allow it as it will allow developers extending it to utilize everything the DatePicker component has to offer: filtering which dates are including, highlighting dates, onSelect events, enabling month/year dropdowns, etc.
There was a problem hiding this comment.
In this case, let's remove passing the extra args to the TimePicker
This PR refactors the
PostScheduleeditor component to make the date and time picker available as components throughout Gutenberg. You can call the date and time picker together viaDateTimePickeror individually viaDatePickerandTimePickerrespectively. AsDatePickerutilizes the React DatePicker library, the spread operator is used to support passing arguments not directly defined by these components.