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

TimeInput: Expose as subcomponent of TimePicker #63145

Merged
merged 6 commits into from
Jul 31, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

### Enhancements

- `TimeInput`: Expose as subcomponent of `TimePicker` ([#63145](https://github.com/WordPress/gutenberg/pull/63145)).
- `RadioControl`: add support for option help text ([#63751](https://github.com/WordPress/gutenberg/pull/63751)).

### Internal
Expand Down
3 changes: 1 addition & 2 deletions packages/components/src/date-time/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,7 @@
*/
import { default as DatePicker } from './date';
import { default as TimePicker } from './time';
import { default as TimeInput } from './time-input';
import { default as DateTimePicker } from './date-time';

export { DatePicker, TimePicker, TimeInput };
export { DatePicker, TimePicker };
export default DateTimePicker;
36 changes: 0 additions & 36 deletions packages/components/src/date-time/stories/time-input.story.tsx

This file was deleted.

17 changes: 17 additions & 0 deletions packages/components/src/date-time/stories/time.story.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import TimePicker from '../time';
const meta: Meta< typeof TimePicker > = {
title: 'Components/TimePicker',
component: TimePicker,
// @ts-expect-error - See https://github.com/storybookjs/storybook/issues/23170
Copy link
Member

Choose a reason for hiding this comment

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

👍

subcomponents: { 'TimePicker.TimeInput': TimePicker.TimeInput },
argTypes: {
currentTime: { control: 'date' },
onChange: { action: 'onChange', control: { type: null } },
Expand Down Expand Up @@ -49,3 +51,18 @@ const Template: StoryFn< typeof TimePicker > = ( {
};

export const Default: StoryFn< typeof TimePicker > = Template.bind( {} );

const TimeInputTemplate: StoryFn< typeof TimePicker.TimeInput > = ( args ) => {
return <TimePicker.TimeInput { ...args } />;
Copy link
Member

Choose a reason for hiding this comment

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

Felt a bit odd to me that the currentTime doesn't seem to work for the TimeInput story, while it does for the default TimePicker one:

Screen.Recording.2024-07-05.at.13.32.06.mov

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, I should've hidden the controls for that story (1bb3f2f).

Copy link
Member

Choose a reason for hiding this comment

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

Another thing I spotted in the latest Chrome - there is a subtle visual difference between the colon of the TimePicker and in the solo TimeInput:

TimePicker:
Screenshot 2024-07-05 at 13 35 33

TimeInput:
Screenshot 2024-07-05 at 13 35 39

You'll spot that for the TimeInput in isolation, the colon appears a bit bigger.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch 😳 Fixed in d94947d

};

/**
* The time input can be used in isolation as `<TimePicker.TimeInput />`. In this case, the `value` will be passed
* as an object in 24-hour format (`{ hours: number, minutes: number }`).
*/
export const TimeInput = TimeInputTemplate.bind( {} );
TimeInput.args = {
label: 'Time',
};
// Hide TimePicker controls because they don't apply to TimeInput
TimeInput.parameters = { controls: { include: [] } };
27 changes: 26 additions & 1 deletion packages/components/src/date-time/time/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import {
validateInputElementTarget,
} from '../utils';
import { TIMEZONELESS_FORMAT } from '../constants';
import { TimeInput } from '../time-input';
import { TimeInput } from './time-input';

const VALID_DATE_ORDERS = [ 'dmy', 'mdy', 'ymd' ];

Expand Down Expand Up @@ -252,4 +252,29 @@ export function TimePicker( {
);
}

/**
* A component to input a time.
*
* Values are passed as an object in 24-hour format (`{ hours: number, minutes: number }`).
*
* ```jsx
* import { TimePicker } from '@wordpress/components';
* import { useState } from '@wordpress/element';
*
* const MyTimeInput = () => {
* const [ time, setTime ] = useState( { hours: 13, minutes: 30 } );
*
* return (
* <TimePicker.TimeInput
* value={ time }
* onChange={ setTime }
* label="Time"
* />
* );
* };
* ```
*/
TimePicker.TimeInput = TimeInput;
Object.assign( TimePicker.TimeInput, { displayName: 'TimePicker.TimeInput' } );
Copy link
Member

Choose a reason for hiding this comment

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

We're now exposing TimePicker.TimeInput as a public subcomponent, right? In that case, shouldn't we be documenting it in the README? This reminds me we also don't document TimePicker and DatePicker at this time - do you know if this is intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct. I'll be adding a readme in the next PR. I guess there is a bit of inconsistency for subcomponent readmes, but I think TimeInput warrants a separate readme because the props are distinct.

do you know if this is intentional?

It doesn't seem like an oversight, because the DateTimePicker readme does mention that DatePicker and TimePicker can be used individually.


export default TimePicker;
1 change: 1 addition & 0 deletions packages/components/src/date-time/time/styles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ export const HoursInput = styled( NumberControl )`
export const TimeSeparator = styled.span`
border-top: ${ CONFIG.borderWidth } solid ${ COLORS.gray[ 700 ] };
border-bottom: ${ CONFIG.borderWidth } solid ${ COLORS.gray[ 700 ] };
font-size: ${ CONFIG.fontSize };
line-height: calc(
${ CONFIG.controlHeight } - ${ CONFIG.borderWidth } * 2
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,20 +18,20 @@ import {
HoursInput,
MinutesInput,
Fieldset,
} from '../time/styles';
import { HStack } from '../../h-stack';
import Button from '../../button';
import ButtonGroup from '../../button-group';
} from '../styles';
import { HStack } from '../../../h-stack';
import Button from '../../../button';
import ButtonGroup from '../../../button-group';
import {
from12hTo24h,
from24hTo12h,
buildPadInputStateReducer,
validateInputElementTarget,
} from '../utils';
import type { TimeInputProps } from '../types';
import type { InputChangeCallback } from '../../input-control/types';
import { useControlledValue } from '../../utils';
import BaseControl from '../../base-control';
} from '../../utils';
import type { TimeInputProps } from '../../types';
import type { InputChangeCallback } from '../../../input-control/types';
import { useControlledValue } from '../../../utils';
import BaseControl from '../../../base-control';

export function TimeInput( {
value: valueProp,
Expand Down
Loading